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

Replace file_name with filename in Effect kwargs #147

Merged
merged 2 commits into from
Nov 28, 2023
Merged

Conversation

hugobuddel
Copy link
Collaborator

file_name as a kwarg does seem to work, but it behaves differently from filename. In particular, with file_name the absolute path of the file is used, and with filename the relative path (or well, the actual given path). I have not investigated why.

The reason for harmonizing is that when generating FITS keywords, we'd like to have them to have the filename as it was given by the user or the irdb, not the absolute path where this file happens to be on this particular installation.

Do we perhaps want to force file_name?

E.g. put an assert in data_container.py instead of a fallback: https://github.com/AstarVienna/ScopeSim/blob/36df1767c2b8143a199d085cdf1af181660c058a/scopesim/effects/data_container.py#L63-L64

        if filename is None and "file_name" in kwargs:
            filename = kwargs["file_name"]

to

        assert "file_name" not in kwargs, "Please use filename instead of file_name"

or maybe a deprecation notice would be better.

But that is a ScopeSim problem; this PR is just making the IRDB consistent.

@hugobuddel
Copy link
Collaborator Author

Oh this also makes the quantum_efficiency naming consistent in METIS, which should perhaps not belong in this PR but is fine as far as I'm concerned.

Copy link
Contributor

@teutoburg teutoburg left a comment

Choose a reason for hiding this comment

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

Certainly useful to have these kwargs harmonized 👍

And as you mentioned this would pave the way to force more consistency with this in ScopeSim, which is also good.

@hugobuddel hugobuddel merged commit a2ece3f into dev_master Nov 28, 2023
9 checks passed
@teutoburg teutoburg deleted the hb/file_name branch February 1, 2024 12:15
teutoburg added a commit to AstarVienna/ScopeSim that referenced this pull request Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants