BUG: Specviz2d to load both s2d and x1d when both provided separately #1717
Conversation
when both provided separately.
pllim
requested review from
duytnguyendtn,
rosteen,
javerbukh,
ojustino,
kecnry and
bmorris3
as code owners
Codecov ReportBase: 87.19% // Head: 87.27% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1717 +/- ##
==========================================
+ Coverage 87.19% 87.27% +0.08%
==========================================
Files 95 95
Lines 10049 10097 +48
==========================================
+ Hits 8762 8812 +50
+ Misses 1287 1285 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| else: | ||
| load_1d = True |
There was a problem hiding this comment.
Why do you set load_1d = True here on the else condition for line 156, i.e. when spectrum_2d is provided? Everything works as expected. Just trying to understand the flow here.
There was a problem hiding this comment.
This line only gets run when spectrum_2d is not provided (None). So I expect you will see the 1D loaded but nothing shown for the 2D viewer. Unless I have misunderstood the code?
There was a problem hiding this comment.
Oh, you mean, line 201? That is to load your separate x1d file. Without it, you see the issue that you reported in the first place. Or did I misunderstood your question?
There was a problem hiding this comment.
I meant the load_1d at line 204. And yeah I think I see what you mean now. It allows the use case when only the 1d is provided but not the 2d. But would this cause an error at line 207, for the case when neither 2d nor 1d file is provided? In that case, looks like load_1d gets set to true, the 1d spectrum tries to load but spectrum_1d is None?
There was a problem hiding this comment.
But why would someone try to load_data without providing the data?
There was a problem hiding this comment.
Good catch on the logic though. I added a check above. Does that look good to you?
There was a problem hiding this comment.
who knows why some people do the things they do. I'm always of the opinion that someone somewhere will try the thing you think no one will try, and thus best safeguard against it. Haha. But this all looks good to me!
| else: | ||
| load_1d = True |
There was a problem hiding this comment.
who knows why some people do the things they do. I'm always of the opinion that someone somewhere will try the thing you think no one will try, and thus best safeguard against it. Haha. But this all looks good to me!
|
@havok2063 , how soon do you need this fix? 😬 |
|
@pllim I don't consider this a critical fix, so I'm fine with this getting merged in on whatever schedule y'all feel is right. We still have to do testing for our update to 3.0.1, and aren't in a rush yet to push to prod. If this gets in a 3.0.2 release, I can easily update our existing PR to bump to the new version. |
|
Thanks, all! Okay, not backporting then. But I created a merge commit, so if we ever change our minds, it is not hard to backport. |
Description
This pull request is to fix #1711 .
I don't know if we want to backport this or not. Feel free to change the milestone if we need to backport this.
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
triviallabel.