Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Spatial Collapsed Spectrum selection to Line Analysis #1583
Add Spatial Collapsed Spectrum selection to Line Analysis #1583
Changes from 13 commits
1715c91
3683d01
27f6916
8867d09
33aab9a
0a05a74
5ca0c5d
9f290eb
21ae02e
f3b6703
1f59f18
98dc53d
0ecf6b0
2dc9faa
1af7936
602b486
e1075c1
5211da7
b52ba14
8bb5178
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd advocate for using the same traitlet names and default text as in model fitting so there is a consistent UX and API for the user between the two. If there are subtle differences that we want to capture in the UI, then the hint might be the easiest place to make that distinction. (Or we can always change model fitting to match if there is a good reason to make changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like to see this addressed/discussed before approving/merging!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to address this in my "review phase" now that the CI is passing. I think I've mellowed on my original intent, so I'm happy to switch it back! Let me do that first and I'll rerequest review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not familiar with model fitting, so maybe @rosteen or @javerbukh would have opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Effectively the distinction I tried to make is between selecting the spatial subset itself, or the spectrum generated from the spatial subset. Effectively, it looks identical on the front end, but it's slightly different here in the backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kecnry I've reverted those changes so they should match; let me know if it's sufficient, or if I missed something!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do see your point, but just really want to strive for self-consistency across plugins, especially with attribute names and options since we're looking to expose these directly in #1401. This still differs from model fittings spatial dropdown (which is the same use-case: acting on the spectrum corresponding to the referenced spatial subset), in that model fitting has "Entire Cube" and this has "Entire Cube Spectrum". I definitely think they should be the same, and personally would vote for "Entire Cube" and add any clarification to the user in the dropdown hint and/or docs. But if others prefer being more verbose, then let's also make the change to model fitting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I misinterpreted your concern to be about the actual traitlets, not realizing you were also concerned about the labeling. I can understand your concern; I'll revert the labeling as well and we can defer this discussion to another day
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I changed the default text/labeling as well. I reworked the
hint
to try and make it explicit. How does that look @kecnry ?