Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow display name changes in data explorer (1702) #1713

Merged
merged 9 commits into from
Jan 19, 2021
Merged

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Dec 3, 2020

A new option was added to the data explorer context menu allowing users to change the display name for a data set. The option opens a window offering 4 options: Keep the existing name, change to the data tile, change to the data file name, or change to any name the user would like. Unit tests were added to check base functionality and catch a few odd edge cases.

This fixes #1701 and closes #1702.

Copy link
Member

@rozyczko rozyczko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, minor functionality related issues raised.

@krzywon
Copy link
Contributor Author

krzywon commented Dec 9, 2020

5.0 ready for testing on Win

# Reset model_item and data to None and close the window
self.model_item = None
self.close()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice feature! We should have these safeguards in more places.

@rozyczko
Copy link
Member

Changes look good, all issues addressed. Ready to merge

@krzywon
Copy link
Contributor Author

krzywon commented Dec 11, 2020

Here are a few comments I've received from Adrian Rennie on the functionality. I'm going to try to implement his suggestions but I'm not sure I will be able to in the PR.

(a) One of the main reasons to use this option is when there are several files that appear, with for example, the same title. In this case, it would be useful to select a number of runs and choose an option for update automatically to e.g. the filename if that is distinctive and the etitle is always the same.

(b) A further comment is that if one changes the name and then changes it back again to what it was previously a '(1)' appears after the name although there is not any extra data set showing in the Data Explorer. Is this intentional?

(c) In terms of future use, I could imagine that people might like to use some automatic selection as the name of other information that might be available. At the moment that is usually rather little but might include the 'Run' number but one might envisage loading other metadata as parameters from a canSAS or nxs file.

@rozyczko
Copy link
Member

(b) A further comment is that if one changes the name and then changes it back again to what it was previously a '(1)' appears after the name although there is not any extra data set showing in the Data Explorer. Is this intentional?

This looks like a bug, the state should know about data filenames at current time.

(c) In terms of future use, I could imagine that people might like to use some automatic selection as the name of other information that might be available. At the moment that is usually rather little but might include the 'Run' number but one might envisage loading other metadata as parameters from a canSAS or nxs file.

This is something we have thought about before and even created a ticket (here or locally in Jira, don't recall).
Modifying the visibility of datasets (not just the check status) based on various criteria (datatype, string in name etc)
Definitely (c) is a separate, large project.

@smk78
Copy link
Contributor

smk78 commented Dec 23, 2020

Have just been testing this branch.
I note that when you plot data, the plot legend always displays the 'data name' irrespective of the 'filename' or any name assigned. However, fitting models use the name assigned, not the data name. For example:
image
image
Here I left the name of the core contrast unmodified, but called the drop contrast 'fred'.
I think this is likely to be very confusing?

@butlerpd
Copy link
Member

The point raised by @smk78 has been addressed in another ticket #1616 and a PR #1745 while c) needs to be turned into another ticket. a) looks like an extra feature request. Is that a quick fix or should it be a separate ticket?

That leaves b) as a bug to address?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
4 participants