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.

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

DM-41953: Implement an adaptive binning option for projection plots in analysis_tools #172

Merged
merged 1 commit into from Dec 19, 2023

Conversation

jrmullaney
Copy link
Contributor

No description provided.

@jrmullaney jrmullaney requested a review from sr525 December 8, 2023 16:28
@jrmullaney jrmullaney marked this pull request as ready for review December 8, 2023 16:29
@jrmullaney
Copy link
Contributor Author

jrmullaney commented Dec 8, 2023

At this stage, the adaptive binning feature has only been added to focalPlanePlot.py. It can be included in skyPlot.py should we wish, but I'd open a new ticket for that.

I've kept the commit history here so you can see my thought processes. I can clean it up using an interactive rebase once it's gone through the review process.

@sr525
Copy link
Collaborator

sr525 commented Dec 11, 2023

I think it would be useful to have something that defaults to points rather than binning in the case of low density. I wonder if it is possible to do both in an efficent manner on the same plot, probably future work.

@jrmullaney
Copy link
Contributor Author

I think it would be useful to have something that defaults to points rather than binning in the case of low density. I wonder if it is possible to do both in an efficent manner on the same plot, probably future work.

Thanks @sr525 for taking a look at this. To clarify - are you ok for me to merge this, then create a new ticket to implement your suggestion for plotting points for low densities? Is this what you meant by "future work"?

doc="Number of bins to use within the effective plot ranges along"
" the spatial directions. If nBins is negative the number of"
" bins is adapted to the source density, with lower densities"
" using fewer bins, but with a minimum of abs(nBins).",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that instead of using negative it would be better to have a config option, something like doUseAdaptiveBinning, that is more explicit and easier to understand at a glance. Then the normal bins config can stay the same and be used for both.

@jrmullaney jrmullaney force-pushed the tickets/DM-41953 branch 2 times, most recently from b83490c to 1f1cc34 Compare December 19, 2023 13:38
@jrmullaney jrmullaney merged commit 3c3afe3 into main Dec 19, 2023
8 checks passed
@jrmullaney jrmullaney deleted the tickets/DM-41953 branch December 19, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants