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

Fix model fitting plugin bugs #1176

Merged
merged 4 commits into from
Mar 18, 2022
Merged

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Mar 17, 2022

Fixes #1144, as well as a bug I noticed that doesn't have an issue, where the data dropdown menu wasn't properly getting populated with spatial subsets when they were created.

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.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a change log needed? If yes, is it added to CHANGES.rst?
  • Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

# so it's not required that we match any specific ids for that case.
# However, if the msg is not none, check to make sure that it's the
# viewer we care about.
if msg is not None and msg.viewer_id != self._viewer_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, and in fact it was breaking it because now it's listening for messages that don't have a viewer_id attached. This was a minuscule performance improvement to not update the data dropdown on irrelevant AddData messages.

@pllim pllim added this to the 2.4 milestone Mar 17, 2022
@pllim pllim added bug Something isn't working cubeviz specviz labels Mar 17, 2022
@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #1176 (a9e4306) into main (be1d987) will increase coverage by 0.00%.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##             main    #1176   +/-   ##
=======================================
  Coverage   76.37%   76.38%           
=======================================
  Files          87       87           
  Lines        6723     6724    +1     
=======================================
+ Hits         5135     5136    +1     
  Misses       1588     1588           
Impacted Files Coverage Δ
...igs/default/plugins/model_fitting/model_fitting.py 31.29% <50.00%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be1d987...a9e4306. Read the comment docs.

CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
@rosteen
Copy link
Collaborator Author

rosteen commented Mar 17, 2022

Not sure how I messed up the PR number in the changelog, good catch 😵

@camipacifici
Copy link
Contributor

I don't get anymore the "ValueError: No data found with the label 'Subset 1'" when trying to fit the spectrum from a collapsed spatial subset!

@pllim
Copy link
Contributor

pllim commented Mar 18, 2022

@camipacifici said on Slack: It still chokes when selecting a sub-subset (as expected right?).

Can someone please confirm whether this is out of scope here or not? Thanks!

@rosteen
Copy link
Collaborator Author

rosteen commented Mar 18, 2022

It's not clear to me what use case is failing, I assume it's the not-actually-intended use case and is thus out of scope. Selecting a spectral subset to apply to the collapsed spectrum of a spatial subset works.

rosteen and others added 4 commits March 18, 2022 11:44
Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

seems like a straightforward fix for now. We can handle the subset styling and not allowing mixing spectral and spatial subsets in future work.

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Approving by proxy since Cami okayed this. :shipit:

@pllim pllim merged commit 07b1605 into spacetelescope:main Mar 18, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cubeviz: model fitting of spatially collapsed cube fails
4 participants