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

Feature/images refactor #218

Merged
merged 25 commits into from
Feb 21, 2025
Merged

Feature/images refactor #218

merged 25 commits into from
Feb 21, 2025

Conversation

Jammy2211
Copy link
Owner

Refactors image output such that:

  • Removes config customization on the level of individual figures, as this made configs too complex and can be achieved via AggretorImages.

  • Outputs .fits files containing key images as .fits output of key information is key, with these grouped packages suited to AggregatorFits object.

  • Creates a nasty new function in MultiFigurePlotter for outputting .fits groups via Plotter interface, quite hacky but works for now.

@Jammy2211 Jammy2211 requested a review from rhayes777 February 21, 2025 10:20
Copy link
Collaborator

@rhayes777 rhayes777 left a comment

Choose a reason for hiding this comment

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

Nice that's a lot cleaner

Comment on lines +389 to +391
galaxy_plotter.figures_1d_decomposed(image=True)
galaxy_plotter.figures_1d_decomposed(convergence=True)
galaxy_plotter.figures_1d_decomposed(potential=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Often passing a boolean to a function like this is a code smell indicating that you should have multiple separate functions instead

"normalized_residual_map",
"chi_squared_map",
],
# tag_list=[name for name, galaxy in galaxies.items()],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commented code?

multi_plotter.output_to_fits(
func_name_list=["figure_2d"] * len(multi_plotter.plotter_list),
figure_name_list=[None] * len(multi_plotter.plotter_list),
# tag_list=[name for name, galaxy in galaxies.items()],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

)

multi_plotter.output_to_fits(
func_name_list=["figure_2d"] * len(multi_plotter.plotter_list),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assign len(multi_plotter.plotter_list) to a variable to avoid repetition


multi_plotter.output_to_fits(
func_name_list=["figure_2d"] * len(multi_plotter.plotter_list),
figure_name_list=[None] * len(multi_plotter.plotter_list),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assign len(multi_plotter.plotter_list) to a variable to avoid repetition

#
# multi_plotter.output_to_fits(
# func_name_list=["figures_2d"] * len(multi_plotter.plotter_list),
# figure_name_list=[
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commented code

"dirty_normalized_residual_map",
"dirty_chi_squared_map",
],
# tag_list=[name for name, galaxy in galaxies.items()],
Copy link
Collaborator

Choose a reason for hiding this comment

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

# )
#
# assert visibilities.shape == (5, 5)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Commented

@Jammy2211 Jammy2211 merged commit a9157b9 into main Feb 21, 2025
4 of 8 checks passed
@Jammy2211 Jammy2211 deleted the feature/images_refactor branch March 24, 2025 19:48
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