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

Imperative rework+ #9

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

queue-miscreant
Copy link
Contributor

@queue-miscreant queue-miscreant commented Aug 30, 2024

Lots of changes:

  • Added type annotations where possible. No Mypy complaints reported.
  • Refactored DColor class to add imperative style plotting.
    • See the part of my comment on rewrite / restructure / push towards better maintainability #5, less the part about matplotlib colormaps
    • This mostly applies to how arguments are supplied to the constructor. Only the plotted function, color map (see below), and sample counts are specific to the object, while arguments applied to the axes are placed in DColor.plot
    • Added wrapper function dcolor, which is a smart constructor for the class that also plots the contents.
    • Axes limits, if not specified, are pulled from the current values on the figure
    • Axes coordinates correspond to data coordinates, rather than image dimensions
    • Add gridlines and x/y axes
    • Add \Re and \Im labels to axes
    • Minor tweaks regarding the imshow call
  • Refactored hsvcolor.py and rgbcolor.py into color_maps.py as pure functions
    • Renamed names of coloring functions to ones that indicate their function:
      • magnitude_oscillating (default)
      • raw_magnitude_oscillating (HSV). Note that this produces identical output for me to magnitude_oscillating, despite no code change to the underlying color conversion.
      • green_magnitude (RGB)
  • Improved example script
    • Generates images in the README. Conseqeuently, these files have been renamed and correspond to the binary files in the diff.
    • Use argparse to plot specific examples
    • Use LaTeX titles

For visual reference, here's a quick comparison between the default outputs before and after:
master
raw_magnitude_oscillating_algebraic_3

Check the diff for the other two (especially HSV/raw_magnitude_oscillating). Personally, I'd like to get rid of the duplicated functionality, but not before having a plan to work with matplotlib colormaps.

Might have gone a little overboard, but I haven't added any truly new functionality aside from the improvements to how plots are displayed.

@queue-miscreant
Copy link
Contributor Author

Coming back to this I realize there are a couple of things that I could have cut out to make this easier; e.g. separating the diagram improvements from the actual imperative rework from the example script improvements. Main reason for the last one being rolled into this is because I didn't want to leave the script broken between PRs, but I wanted a way to generate images for the README without just taking screen captures.

I can break this PR down if it would make it easier on you.

@hernanat
Copy link
Owner

hernanat commented Sep 11, 2024

Coming back to this I realize there are a couple of things that I could have cut out to make this easier; e.g. separating the diagram improvements from the actual imperative rework from the example script improvements. Main reason for the last one being rolled into this is because I didn't want to leave the script broken between PRs, but I wanted a way to generate images for the README without just taking screen captures.

I can break this PR down if it would make it easier on you.

this is really on me for not having written contribution guidelines and such a long time ago. I do think smaller PRs are easier to review and reduce reviewer fatigue.

I would appreciate it if it was broken up, however I realize you've also been putting a lot of effort into this stuff and so I am happy to review this as-is (probably tomorrow ... which I guess is actually today at this point) since I didn't have the guidelines in the first place.

On another note, if you are interested in becoming a co-maintainer of the project we should chat -- you seem to be interested in it and I'd be happy to work together as maintainers if that is something you'd be open to. My email is in the README (against my better judgement ~8 years ago) if you'd want to have a conversation.

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.

2 participants