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

Implementation of read-in for general plot settings #27

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

smayotte
Copy link
Contributor

@smayotte smayotte commented Feb 1, 2022

Here is the implementation I came up with to read-in general plot settings, such as filename, file extension, or figure size. It should work for all available methods to plot via command line (--plotconfig and --plot/--plotall). For --plotconfig the read in is automatic, for --plot/--plotall the settings have to be submitted via --plotsettings.

…option and figure size) for all plot options (--plotconfig and --plot/--plotall)
@smayotte smayotte self-assigned this Feb 1, 2022
@smayotte smayotte added the enhancement New feature or request label Feb 1, 2022
@smayotte smayotte linked an issue Feb 1, 2022 that may be closed by this pull request
3 tasks
…lor/colormap, corrected default behaviors if no plot-settings are chosen when calling --plot/plotall
…ched to hexbins for 2D-histograms because they look better. Adjustments for show-plot still needed!
@Areustle
Copy link
Contributor

Okay I've gotten a chance to look into these commits. They're mostly good work, and do implement the job you hoped to do. There are however 2 high-level issues that I have with this, both of which suggest a rework.

  1. Code Duplication.
    This is a pretty common occurrence when new features are added to an existing project. The mechanisms for reading and managing control-flow as a consequence of the plot-kwargs options is repeated multiple times throughout the various local_plots.py. For instance the logic to update colors and determine whether to plt.show() the pop-up plot. I can see how that grew organically out of what I had written before, but now that there's more logic involved we need a different strategy, one that doesn't violate DRY. If we ever need to update that logic we need to change the same thing in multiple places, or risk inconsistencies.

  2. Responsibility Distribution.
    Related to the first point, we've now got a situation where the responsibility for handling plotting is distributed across multiple logical actors, meaning that no one function or class is responsible for the act of plotting. This will make adding new plots in the future conceptually difficult because the logic is spread across the codebase.

Otherwise, the lower-level details are good. I like what you've done with the matplotlib hexbin methods, and applaud your use of black, and flake8 in these commits 👏.

Proposed solution:

Let's refactor and create a separate plotting class or a closure function that consolidates this responsibility in a single logical entity. Then we'll rework the src/nuspacesim/utils/decorators.py decorator functions to accept this class and make plotting decisions internally, based on the plot_config.ini. Probably the right thing to do in that regard is to remove those plotting decorators from decorators.py and add them into a fully refactored src/nuspacesim/utils/plots.py

Copy link
Contributor

@Areustle Areustle left a comment

Choose a reason for hiding this comment

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

See above.

@smayotte
Copy link
Contributor Author

Yes, I think you're completely right! Thinking about it a bit the last two weeks, especially with regard to making even more plots available, the current implementation is too repetitive.
I think a class/closure function as you mentioned is the way to go. I have not worked too much with those prior to working on nuspacesim, so it might take me a bit of time to rework the code. But I'm sure I can do it and excited to learn something new!

@smayotte
Copy link
Contributor Author

smayotte commented May 9, 2022

I am not sure if this is anywhere close to what you had in mind and it might need some more work. Please let me know what you think and I can make changes!

@smayotte
Copy link
Contributor Author

I made some changes to default values and added the --plotsettings flag that can be used when making plots with --plot instead of --plotconfig. There was also an issue with the binwidth being negativ when plotting profiles that is now fixed.
The unit testing is still giving a lot of problems and needs some thorough evaluation...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SaveFig plot naming scheme and auto_increment
2 participants