Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DOC: Add warning about surface brightness in Simple Aperture Photometry plugin #2261

Merged
merged 2 commits into from
Jun 23, 2023
Merged

DOC: Add warning about surface brightness in Simple Aperture Photometry plugin #2261

merged 2 commits into from
Jun 23, 2023

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Jun 21, 2023

Description

This pull request is to clarify what to expect from Simple Aperture Photometry plugin when your data is in surface brightness unit.

Screenshot 2023-06-21 152047

Change log entry

  • Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
    list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
    should add a no-changelog-entry-needed label.

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 milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

@pllim
DOC: Add warning about surface brightness
98b74ef 
in Simple Aperture Photometry plugin.

I am beginning to think the Simple in plugin name no longer applies.
@pllim pllim added documentation Explanation of code and concepts imviz no-changelog-entry-needed changelog bot directive plugin Label for plugins common to multiple configurations labels Jun 21, 2023
@pllim pllim added this to the 3.6 milestone Jun 21, 2023
@codecov
Copy link

codecov bot commented Jun 21, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (0fdb3bd) 91.37% compared to head (5b97fc5) 91.37%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2261   +/-   ##
=======================================
  Coverage   91.37%   91.37%           
=======================================
  Files         150      150           
  Lines       16862    16862           
=======================================
  Hits        15407    15407           
  Misses       1455     1455

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

camipacifici
camipacifici reviewed Jun 21, 2023
View reviewed changes
Copy link
Contributor

@camipacifici camipacifici left a comment

Choose a reason for hiding this comment

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

How about this:
If your data is in surface brightness units and pixels on the image have varying sky area, you should first convert your data from surface brightness to flux units before using this plugin. This is because, for performance reasons, the plugin multiplies by the area after the aperture sum is calculated.
Fixing a typo and slightly streamlined.

@pllim @camipacifici
DOC: Improve verbiage.
5b97fc5 
Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com>
@pllim
Copy link
Contributor Author

pllim commented Jun 21, 2023

@camipacifici , I applied your suggestion. Thanks! Please re-review.

rosteen
rosteen approved these changes Jun 23, 2023
View reviewed changes
Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

I trust you and @camipacifici on this one, sounds fine to me!

@pllim pllim merged commit d5fbd70 into spacetelescope:main Jun 23, 2023
@pllim pllim deleted the aper-phot-moar-warning branch June 23, 2023 14:21
rosteen added a commit that referenced this pull request Jul 5, 2023
@maartenbreddels @duytnguyendtn
@pllim @javerbukh @camipacifici @Jenneh @rosteen @mariobuikhuizen @kecnry
POC: Attempt to package jdaviz in Pyinstaller (take 2) (#1960)
46be84f 
* feat: pyinstaller

* codesign osx

* debug: try without codesign

* does not need arguments

* use hooks

* also download the app

* use branch of pyinstaller

* better hooks

* pin to 5.11

Otherwise we need to do at least:
+        rm -rf standalone/dist/jdaviz.app/Contents/MacOS/jedi/third_party/typeshed/stdlib/
+        rm -rf standalone/dist/jdaviz.app/Contents/MacOS/**/*.dist-info

* fix: maintain symlinks by zipping, upload-artifact does not support it

* GHA logic?

* code sign on gha

* run notary tool on gha

* fix: redo codesign after modifications

* fix: reorder zipping and notary step

* remove invalid symlink

* make sure the program executes

* fix path of entitlements file

* add comment for hint with the osx fix

* gpt assisted way of running the log tool on failure

* fix syntax error

* fix path

* comments for the future

* fix uuid parsing and re-zip the app after notary step

* fix: use ditto instead of zip

from https://developer.apple.com/forums/thread/116831

zip might cause issues with utf8 or metadata.

* comments for the future

* fix uuid for notary step

* upload different artifact for osx

* cleanup spec file and hooks, no more __pycache__ files should be included

* ci: do not cancel on failure

* fix: mistune 3.0 needs this

* make dmg instead of zip for osx

* BUG: Fix mouseover behavior in Cubeviz
spectrum viewer when spatial subset is present.

Co-authored-by: Duy Nguyen <duytnguyendtn.open@gmail.com>

* Remove change log from #2258

because the bug only affects unreleased code

[ci skip] [rtd skip]

* Deprecate get_subsets_from_viewer

* Missing region index

* Fix subset args

* Remove get_data_from_viewer from imviz viewer tests

* Remove get_data_from_viewer from mosviz data loading test and sub hardcoded viewer ref names

* Mosviz test update image truth class

* Fix incorrect viewer ref

* Set Mos2Dviewer data statistic to None by default

* Non-existent data check

* Properly deprecate getters

* Properly check for valueError on non-existent label

* Update Specviz get_data_from_viewer test

* Rename "subset_to_apply" to "spectral_subset

* Codestyle

* Fix docs wording

Co-authored-by: Jesse Averbukh <javerbukh@gmail.com>

* Catch missed code, fix bug

* Retain Mosviz get_data behavior
and fix change log

* Undo bad diff

* MNT: Add .mailmap
so git shortlog -es gives sane listing.

* DOC: Add warning about surface brightness in Simple Aperture Photometry plugin (#2261)

* DOC: Add warning about surface brightness
in Simple Aperture Photometry plugin.

I am beginning to think the Simple in plugin name no longer applies.

* DOC: Improve verbiage.

Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com>

---------

Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com>

* FEAT: Annulus draw tool for Imviz (#2240)

* FEAT: Annulus draw tool for Imviz.

TODO: Need to fix icon.

TST: Add tests.

[ci skip] [rtd skip]

* Pull in #2204 into this PR
and bump upstream pins

* Changed function name
upstream in glue-astronomy during review process

* Proper annulus icon from J. Kotler

Co-authored-by: Jennifer Kotler <jennlkotler@gmail.com>

* Avoid error traceback with bad annulus radii

* Disable recentering for annulus

* Fix test failure
because not sure why spectral region is using Imviz centering method
but okay.

---------

Co-authored-by: Jennifer Kotler <jennlkotler@gmail.com>
Co-authored-by: Ricky O'Steen <rosteen@stsci.edu>

* MNT: Temporarily pin voila<0.5 (#2269)

* MNT: Temp pin voila<0.5
because voila-template is incompatible, see #2268

* TST: Disable voila dev in test matrix

* TST: Ignore DeprecationWarning from asteval (#2274)

* TST: Ignore DeprecationWarning from asteval.
For example https://github.com/newville/asteval/issues/120

* Also ignore FutureWarning from asteval

* Add doc for windows latency issue

* Deprecate load_spectrum

* Update tests and docs from load_spectrum

* Docstring suggestions

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

* Changelog

* Changelog

* Button to export Cubeviz movie (#2264)

* WIP: Backend API to write movie file
but got stuck with bqplot not cooperating after first frame.

[ci skip] [rtd skip]

* fix: bqplot stuck after the first frame of the movie

We have to run the loop outside the main thread; otherwise, the
processing of messages from the frontend is blocked, causing the
message with the first image to never be received. The "save_image"
method can only save the next image after the previous image is
received.

* Fix typo, this works now from the API.

[ci skip] [rtd skip]

* Add frontend and tests

* Add user doc

* DOC: Add note about standalone app
saving file into weird places.

* Address some review comments

* Expose FPS and fix test

* DOC: Baby Shark roundtrip
as promised.

* DOC: Ellie said more shark!

* Vue.js style improvements

[ci skip] [rtd skip]

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

* Display pls install msg in plugin

* Address review comments

* Fix test

* Fix path resolution in standalone app

* Improve frontend validation and access.

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

* Disable video for spectrum-viewer in GUI
and add comment to Slice.

* Add tooltip to kill switch
but it only shows when activated

* Rename kill with something less scary

* Hide stop button
since disabling is not obvious enough, also moar tooltip

* Disable the whole movie menu for spectrum

viewer in Cubeviz [ci skip] [rtd skip]

Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>

* Fix RTD warnings about invalid prop
and remove unnecessary frontend check now that Ricky's suggestion is accepted.

---------

Co-authored-by: Mario Buikhuizen <mariobuikhuizen@gmail.com>
Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>

* Add functional but ugly launcher

* Use launcher notebook if neither config nor filepath is specified

* Codestyle

* Add margins on left and right to avoid cutoff in notebook

* Support launcher from cli

* Codestyle

* Remove URI from path text until implemented

* Fix standalone bug

* Specify --layout= as new required cli syntax

* Remove unused import

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

* Update readme to show required layout flag

* Changelog

* Changelog

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

* Changelog

* Update .github/workflows/standalone.yml

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

* Update .github/workflows/standalone.yml

* Update to use config launcher

* Moved changelog entry to new section

---------

Co-authored-by: Duy Nguyen <duytnguyendtn.open@gmail.com>
Co-authored-by: Pey Lian Lim <2090236+pllim@users.noreply.github.com>
Co-authored-by: Duy Tuong Nguyen <dtn5ah@virginia.edu>
Co-authored-by: Jesse Averbukh <javerbukh@gmail.com>
Co-authored-by: Camilla Pacifici <camilla.pacifici@gmail.com>
Co-authored-by: Jennifer Kotler <jennlkotler@gmail.com>
Co-authored-by: Ricky O'Steen <rosteen@stsci.edu>
Co-authored-by: Mario Buikhuizen <mariobuikhuizen@gmail.com>
Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rosteen rosteen rosteen approved these changes

@camipacifici camipacifici camipacifici approved these changes

@larrybradley larrybradley Awaiting requested review from larrybradley

@duytnguyendtn duytnguyendtn Awaiting requested review from duytnguyendtn

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

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

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

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

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

Assignees
No one assigned
Labels
documentation Explanation of code and concepts imviz no-changelog-entry-needed changelog bot directive plugin Label for plugins common to multiple configurations Ready for final review
Projects
None yet
Milestone
3.6
Development

Successfully merging this pull request may close these issues.
3 participants
@pllim @camipacifici @rosteen