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/visualize refactor #96

Merged
merged 216 commits into from
Apr 24, 2023
Merged

Conversation

Jammy2211
Copy link
Owner

Cleans up visualization code to make default images appear closer to publication quality.

The subplot_fit we use to inspect model-fits now have 9 panels, so we do not have to alternative between subplot_of_planes:

image

Comment on lines +4 to +6
if TYPE_CHECKING:
from autogalaxy.galaxy.galaxy import Galaxy
from autogalaxy.interferometer.fit_interferometer import FitInterferometer
Copy link
Collaborator

Choose a reason for hiding this comment

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

So much better

@@ -444,6 +440,9 @@ def output_or_check_figure_of_merit_sanity(
if os.environ.get("PYAUTOFIT_TEST_MODE") == "1":
return

if conf.instance["general"]["test"]["bypass_figure_of_merit_sanity"]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol why do we want to bypass this?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Not often, but we had some science runs messed up by it so its good to have an override.

Comment on lines 16 to 19
adapt_galaxy_image_path_dict=None,
adapt_model_image=None,
adapt_galaxy_visibilities_path_dict=None,
adapt_model_visibilities=None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Types would be nice

Copy link
Collaborator

Choose a reason for hiding this comment

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

I appreciate you just renamed these though


self.search_bc_cls = search_bc_cls or af.DynestyStatic
self.search_bc_dict = search_bc_dict or {
self.search_pix_cls = search_pix_cls or af.DynestyStatic
Copy link
Collaborator

Choose a reason for hiding this comment

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

Types are immutable so you could instead define this as a default argument

self,
hyper_galaxy_image_path_dict: {str, aa.Array2D},
hyper_model_image: aa.Array2D,
adapt_galaxy_image_path_dict: {str, aa.Array2D},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dict[str, aa.Array2D]

Comment on lines 117 to 120
for path, galaxy in self.path_galaxy_tuples:
hyper_galaxy_visibilities_path_dict[path] = self.visibilities_galaxy_dict[
adapt_galaxy_visibilities_path_dict[path] = self.visibilities_galaxy_dict[
path
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be a dict comprehension

@@ -100,6 +98,13 @@ def cls_list_from(self, cls: Type) -> List:
def galaxies_with_cls_list_from(self, cls: Type) -> List[Galaxy]:
return list(filter(lambda galaxy: galaxy.has(cls=cls), self.galaxies))

@property
def adapt_galaxies_with_pixelization_image_list(self) -> List[aa.Array2D]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docs?

Copy link
Owner Author

Choose a reason for hiding this comment

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

One day.

Comment on lines +60 to +73
def tangential_critical_curves(self):
return self.load(
value=self._tangential_critical_curves, name="tangential_critical_curves"
)

@property
def radial_critical_curves(self):
return self.load(
value=self._radial_critical_curves, name="radial_critical_curves"
)

@property
def tangential_caustics(self):
return self.load(value=self._tangential_caustics, name="tangential_caustics")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docs?

"""
super().__init__(centre=centre, ell_comps=(0.0, 0.0))

self.m = 4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does this number come from?

Comment on lines +7 to +29
setup = ag.legacy.SetupAdapt(search_pix_cls=None, search_pix_dict=None)
assert setup.search_pix_cls == af.DynestyStatic
assert setup.search_pix_dict == {
"nlive": 50,
"sample": "rwalk",
}

setup = ag.legacy.SetupAdapt(
search_pix_cls=af.DynestyDynamic,
search_pix_dict={"hello": "there"},
)
assert setup.search_pix_cls == af.DynestyDynamic
assert setup.search_pix_dict == {"hello": "there"}

setup = ag.legacy.SetupAdapt(search_noise_cls=None, search_noise_dict=None)
assert setup.search_noise_cls == af.DynestyStatic
assert setup.search_noise_dict == {"nlive": 50, "sample": "rwalk"}

setup = ag.legacy.SetupAdapt(
search_noise_cls=af.DynestyDynamic, search_noise_dict={"hello": "there"}
)
assert setup.search_noise_cls == af.DynestyDynamic
assert setup.search_noise_dict == {"hello": "there"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be four separate tests

@Jammy2211 Jammy2211 merged commit 8ed1e89 into feature/coverage Apr 24, 2023
7 checks passed
@Jammy2211 Jammy2211 deleted the feature/visualize_refactor branch June 12, 2023 15:36
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

3 participants