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

New simulation model #761

Merged
merged 243 commits into from
Mar 7, 2024

Conversation

GernotMaier
Copy link
Contributor

@GernotMaier GernotMaier commented Jan 3, 2024

This is a very large pull request with several major changes:

  • changing names of telescopes to official CTAO naming (e.g., MSTN-01). This required a re-writing of the model database and many changes in the code
    • open point: update all docstrings and documentation for new telescope naming
  • new organisation of model parameters with a base model_parameter class and to specialized classes called telescope_model and site_model (allows to add in future e.g., illuminator_model classes)
  • reading of simulation model parameters from json files (either from file from from repository)
  • all site and telescope-related parameters are now in the database (or in the simulation model repository)
  • additional changes for more consistent naming (e.g., in db instead of units use unit (as in astropy)

Given that telescope naming has changed, this PR required a lot of name-but-not-functional changes in code, tests, and data files used.

Temporary items to be fixed best in a future PR:

  • simulation_model is expected to be on GitHub (planned to be moved to CTAO gitlab)
  • CIs for unit- and integration tests required the cloning of the simulation model (see here and here). This should go when all parameters are in the simulation model database
  • old/new names are currently duplicated in names.py (see here. This also requires changes in the database.
  • missing documentation updates (this is probably a more general issue)

Suggestion for review:

  • a lot of files changed.
    • suggest first to do the data files in tests/resources: changes are in telescope names (e.g., from LST-1 to LSTN-01) and removal of corsika-related parameters from the metadata header (now included in db_handler)
    • data/parameters/corsika_parameters.yml : all site/telescope related parameters are no in the DB
  • simple changes: db_handler is now in db/db_handler (as there are more db-related modules). This obviously changes all imports on db_handler
  • for actual code review: best maybe to start with names.py, then db_handler and then the model parameter classes (model_parameter, telescope_model, site_model).
  • after that, look at the rest of the modules and tests (lots of little and annoying changes)

@GernotMaier GernotMaier self-assigned this Jan 3, 2024
Copy link
Contributor

@orelgueta orelgueta left a comment

Choose a reason for hiding this comment

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

Very nice!

I made just a few comments and only one of them worries me a bit (about the Structure/Camera readout from the DB).

simtools/camera_efficiency.py Outdated Show resolved Hide resolved
simtools/data_model/data_reader.py Outdated Show resolved Hide resolved
simtools/db/db_handler.py Show resolved Hide resolved
simtools/db/db_handler.py Show resolved Hide resolved
except KeyError:
pass

def _load_telescope_list(self, table):
def initialize_array_layout(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I see it fixed (doesn't start with _)

simtools/model/site_model.py Show resolved Hide resolved
Simtel site parameters as dict

"""
return super().get_simtel_parameters(telescope_model=telescope_model, site_model=site_model)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this at all? if you call the function from the base class without any changes anyway, you can just remove it and you will get the function from the base class

"SSTS": {"site": "South", "observatory": "CTAO", "class": "telescope"},
"SCTS": {"site": "South", "observatory": "CTAO", "class": "telescope"},
# calibration devices
"ILLN": {"site": "North", "observatory": "CTAO", "class": "calibration"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Since to write these dictionaries you looked these up, perhaps you can add comments on each of this to say what it is cause I definitely cannot tell from the short name. But only if you can do it off the top of your head, no need to put effort into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean what ILLN North means? I think I remember.

I anyway plan to remove all those lines and move them into the json schema (some parts are already duplicated, e.g., the list of telescopes) - names.py will read then the schema files to get the naming dicts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, ILLN, RLDN, STPN, etc. I don't remember what all of them means and I think it would be useful to have the non-abbreviated names as well. Also if it's in the schema files it would be useful.

simtools/utils/names.py Outdated Show resolved Hide resolved
simtools/utils/names.py Show resolved Hide resolved
@GernotMaier
Copy link
Contributor Author

Screenshot 2024-03-06 at 16 00 35

Github does not allow me to reply on that: 'fixed'.

@GernotMaier
Copy link
Contributor Author

Screenshot 2024-03-06 at 16 21 20

Propagated this to issue #828 (and again I cannot answer to this comment)

@GernotMaier
Copy link
Contributor Author

Screenshot 2024-03-06 at 16 24 35

I don't know enough about python to answer this question. However, I find that this line increases readability of the code dramatically and would prefer to keep it.

@GernotMaier
Copy link
Contributor Author

@orel - thanks for the review!! I've addressed all of your comments.

I will need your help on the db_handler:_get_model_parameters_mongo_db for the selection of the applicable parameters. What is wrong with the implementation now? Is it inefficient, as we get the full telescope model instead of camera or structure only? Or is there another issue which I don't see?

@orelgueta
Copy link
Contributor

Changes look good. Let's discuss the thing about the camera/optics in a quick call instead, it would be quicker.

The only other point here is the super() function which you claim helps readability. I find that surprising cause I suggest to just remove that function completely, so there's nothing to read =)
Perhaps I am wrong though, I am not an expert on python inheritance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment