-
Notifications
You must be signed in to change notification settings - Fork 0
New option for circular contours with radii sent as parameters #25
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
Merged
Merged
Changes from all commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
92cfc29
deprecate log_func
mlincett 01931ce
<bot> update requirements-examples.txt
wipacdevbot bfdf70e
<bot> update requirements-tests.txt
wipacdevbot 036a620
<bot> update requirements.txt
wipacdevbot 478b2af
deprecate upload-func
mlincett 36b94d3
deprecate further slack posting logic
mlincett ff212e4
further remove deprecated logic
mlincett 5d23c1b
assume we always save plots
mlincett e8cee41
single source of truth for inches/dpi
mlincett 6b6840d
remove unused vars
mlincett d3f4431
remove import io
mlincett 1ee865a
update example
mlincett cda9860
de-type plotting function
mlincett 3d6747f
some notes from mypy
mlincett b0730dc
mypy readiness step
mlincett 929b185
<bot> update setup.cfg
wipacdevbot dabfff5
<bot> update requirements-examples.txt
wipacdevbot d80577d
<bot> update requirements-tests.txt
wipacdevbot 004881f
<bot> update requirements.txt
wipacdevbot a773efe
Testing conection branch skymist
G-Sommani 612494d
Fixed minor bug in result.py, function make_plot_zoomed()
G-Sommani 4117ce8
Fixed minor bug in result.py, function make_plot_zoomed(), part 2
G-Sommani 438a1d7
Testing conection branch skymist Part 2
G-Sommani 2761d60
New boolean value to identify rude events
G-Sommani d460449
Exploring new contours for rude events Part 1
G-Sommani 35ccbcf
Exploring new contours for rude events Part 2
G-Sommani 9142568
Exploring new contours for rude events Part 3
G-Sommani f26a5f3
Exploring new contours for rude events Part 4
G-Sommani d93f254
Exploring new contours for rude events Part 5
G-Sommani 586d724
Exploring new contours for rude events Part 5
G-Sommani 7c44247
Exploring new contours for rude events Part 6
G-Sommani 2368ce3
Exploring new contours for rude events Part 7
G-Sommani 1017e00
Implemented contours for rude events
G-Sommani 988fb21
all changes (minor)
G-Sommani b897ee9
<bot> update requirements-examples.txt
wipacdevbot d4725f7
Solve issues for merge
G-Sommani ce5d6a9
Moved circular_contour() to class level
G-Sommani 2c23451
Missing self in circular_contour() on class level
G-Sommani 3c82025
boolean parameter 'is_rude' changed in 'circular'. Added 50% and 90% …
G-Sommani b78a768
Improved handling of circular contours with numpy
G-Sommani 08b5165
Fixed error in calling np.dstack()
G-Sommani 7488e49
Fixing bugs after implementation of np.dstack()
G-Sommani d3819d0
circular_contour() as static method
G-Sommani File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
While this is acceptable to me at this stage, I don't like very much the idea of an ever-growing list of (boolean) arguments in order to control the variant of the plotting.
This leads to a situation where we have 2^n combinations of settings and most of those are not meaningful or have undefined behaviour.
For example, passing
circular=Truewould make the value ofsystematicsirrelevant.At some point we may want to define the plotting variants in a more structured way (as part of #22 ).
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 also wonder if it's possible to merge the functionality here with the
extra_ra/dec/radiusarguments? Those are used for plotting the online splinempe circular contours already. With the additionalerr50/err90arguments that means it's also possible forerr90 < err50which wouldn't be correct.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 think it would be nice to keep the option to have both the online splinempe circular contours and other circularized errors at the same time, so I would keep all the arguments. I can add the raising of an error if err90 is smaller than err50.
Uh oh!
There was an error while loading. Please reload this page.
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.
Yes, there are several scenario we should cover, I tried to put together a list in #26
The existing extra* arguments allow for a single radius at an arbitrary direction, while here we want two radii at the best fit direction.
I am not sure if we should put effort into merging the two functionalities now considering that all would have to be refactored later.
I guess ultimately we want to achieve something of the kind:
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.
Thanks @mlincett for creating the issue. I'm fine with merging this in as it sounds like we're all onboard with improving things later.