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

Jump to bottom

BUG: Fix line analysis results logic #934

Merged
merged 1 commit into from
Oct 15, 2021
Merged

BUG: Fix line analysis results logic #934

merged 1 commit into from
Oct 15, 2021

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Oct 13, 2021

Description

This pull request is to avoid setting "results" in every iteration, but only set it once at the end. Does not change any user facing functionality.

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)?

@pllim pllim added bug Something isn't working no-changelog-entry-needed changelog bot directive labels Oct 13, 2021
@pllim pllim added this to the 2.1 milestone Oct 13, 2021
@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #934 (9d1c209) into main (4c2c3a5) will increase coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #934      +/-   ##
==========================================
+ Coverage   69.37%   69.38%   +0.01%     
==========================================
  Files          69       69              
  Lines        4888     4887       -1     
==========================================
  Hits         3391     3391              
+ Misses       1497     1496       -1
Impacted Files Coverage Δ
...igs/specviz/plugins/line_analysis/line_analysis.py 62.29% <0.00%> (+1.00%) ⬆️

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 b01d538...9d1c209. Read the comment docs.

kecnry
kecnry approved these changes Oct 14, 2021
View reviewed changes
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 straightforward enough and works fine on my end!

jdaviz/configs/specviz/plugins/line_analysis/line_analysis.py
@@ -139,7 +139,5 @@ def _run_functions(self, *args, **kwargs):
temp_result = FUNCTIONS[function](self._spectrum1d)

temp_results.append({'function': function, 'result': str(temp_result)})
self.result_available = True

self.results = []
Copy link
Member

Choose a reason for hiding this comment

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

I've seen this elsewhere with a comment that its to trick the traitlets into forcing an update... is it ok to remove here? Can we remove it everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It still updates fine locally for me with this patch, but I cannot guarantee globally.

If you think this patch is invalid, then feel free to close without merge.

Copy link
Member

Choose a reason for hiding this comment

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

It works fine for me locally, too. Looking at the surrounding code more it looks like it will always be overwriting the default empty list anyways, so I see no reason why it would need to be set to empty a first. I'm more just wondering if we can cleanup other plugins by removing similar logic, but that's clearly out of scope for this PR.

@ojustino
Copy link
Contributor

I don't notice any changes in the UX, which is good. Should we be worried about the codecov warning?

@pllim
Copy link
Contributor Author

pllim commented Oct 15, 2021

Re: codecov -- Adding test for this plugin is out of scope of this PR. 😉

ojustino
ojustino approved these changes Oct 15, 2021
View reviewed changes
Copy link
Contributor

@ojustino ojustino left a comment

Choose a reason for hiding this comment

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

Scope strikes again. OK, then I'll approve.

@pllim pllim merged commit 43976a1 into spacetelescope:main Oct 15, 2021
@pllim pllim deleted the line-analysis-result branch October 15, 2021 15:37
@pllim
Copy link
Contributor Author

pllim commented Oct 15, 2021

Thanks for the reviews!

@pllim
Copy link
Contributor Author

pllim commented Oct 15, 2021

@duytnguyendtn , I don't think I need a JIRA ticket for such trivial fix, do I?

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@kecnry kecnry kecnry approved these changes

@ojustino ojustino ojustino approved these changes

@duytnguyendtn duytnguyendtn Awaiting requested review from duytnguyendtn

@ibusko ibusko Awaiting requested review from ibusko

@javerbukh javerbukh Awaiting requested review from javerbukh javerbukh is a code owner

@rosteen rosteen Awaiting requested review from rosteen rosteen is a code owner

Assignees
No one assigned
Labels
bug Something isn't working no-changelog-entry-needed changelog bot directive Ready for final review specviz
Projects
None yet
Milestone
2.1
Development

Successfully merging this pull request may close these issues.
3 participants
@pllim @ojustino @kecnry