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

show_dashboard duplicates boilerplate code in sonora and phoenix #61

Open
gully opened this issue Jun 21, 2022 · 0 comments
Open

show_dashboard duplicates boilerplate code in sonora and phoenix #61

gully opened this issue Jun 21, 2022 · 0 comments

Comments

@gully
Copy link
Member

gully commented Jun 21, 2022

SonoraGrid and PHOENIXGrid both have a .show_dashboard() method. The two methods share almost the same code duplicated nearly verbatim, possibly as much as 90% identical, with only about 10% differences. This Write Everything Twice (WET) approach appears to violate Don't Repeat Yourself (DRY). show_dashboard constitudes 44% of the lines of code of phoenix.py and 40% of the lines of code for sonora.py, so reducing the duplicate code would stands to make a big impact.

The problem is that the 10% differences are difficult to abstract. In particular, we would need a way to generalize dimensions of grids. That amounts to:

"Add a slider that changes the fourth grid dimension"

...means that we need a pre-requisite goal:

"Abstract the grid dimensions with aliases so that unlike grids can share underlying mechanics."

So for example, we would have something like

grid.first_dimension = grid.teff_points
grid.second_dimension = grid.logg_points
grid.third_dimension = grid.metallicity_points
grid.fourth_dimension = grid.alpha_abundance_points # (Not Implemented yet)

That's easy enough to do, but in practice the corresponding manipulations can be tricky. For example, Sonora and PHOENIX both have ragged/jagged/sparse filling of those dimensions. So we would need some generic mechanism to handle both the abstraction and the "raggedness". That might be straightforward, but for some reason it sounds error-prone too.

@SujayShankarUT and I agree that the path forward is to do some exploratory work in this direction:

  1. Add the fourth dimensions to both grids (i.e. C / O for Sonora #40 and $[\alpha/Fe]$ for Phoenix). The idiosyncracies of these dimensions will reveal themselves and show the righteous path towards abstraction layers.
  2. Add some experimentation in to-what-extent we can abstract the dimensions. So for example, we could implement a show_dashboard method in precomputed_spectrum that the users would never see because it gets overridden instantly by each subclass, but we developers could be experimenting with it behind-the scenes. Then, once it's working, we can simply delete the subclass methods and the parent class method will gracefully take over. This dev strategy will give us a long runaway for experimentation...
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

No branches or pull requests

1 participant