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

Changes to include the possibility of a notebook computing the physical parameters #264

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

olivierilbert
Copy link
Collaborator

I started from the branch Improve-headers-displays which is ready to be merge.
So, this branch already include previous changes compared to the main. It's maybe better to merge Improve-headers-displays before.

I added a notebook to compute physical parameters, starting from the typical example with COSMOS2020.

I realize a problem on the use of the .list files when creating the notebook.
In command lines, it is the absolute path used for the .list file (and in previous versions of the code). But in prepare.py and the unit tests, it was the relative path starting in lephare-data.
It's really convenient to have the .list file within its own directory to change whatever you want in your experiment. We need to keep that. So, I did a modification:

  • If I spot "sed/" in the name, I consider that it is relative to lephare-data
  • If not, I consider that it's an absolute path.
    By doing that, you can still use the tools data_retrieval.py etc, and you can have your file in your own directory.
    The only problem is if someone store the .list file in a directory different from lephare-data and having sed/ in its path. The chances are really low to do that. If we want to not take the risk, we indicate in the doc that the user should have the .list file either in its running directory, either in lephare-data. In such case, it would work.

@olivierilbert olivierilbert marked this pull request as draft December 20, 2024 17:34
@olivierilbert
Copy link
Collaborator Author

I have no idea how to add the notebook in the pre-commit. It makes the tests failing on the distant repository.
Raphael, do you think you can take that in charge?

Notebooks must be empty for the precommit hooks to pass. I think the notebook also has to be in the toc to be executed remotely
@raphaelshirley
Copy link
Member

One issue is that the notebooks muct be empty when committed. if you had precommit installed on your machine it would not have let you commit it with outputs in the notebook but would have modified the file and you would have to recommit it. You can install precommit locally using the advanced install instructions:

pre-commit install

you can then run it automatically when you git commit or seperately with:

pre-commit run --all-files

I think the notebook also has to be added to the toc to execute so I added it alongside the other notebooks in the python-usage toc. Lets see if that helps...

@raphaelshirley
Copy link
Member

Changes to data retrieval seem to be breaking the download in the notebook. We might also see this in the tests if they could run.

@raphaelshirley
Copy link
Member

The documentation is now failing on another notebook at the line:

lp.data_retrieval.get_auxiliary_data(keymap=keymap,additional_files=["examples/COSMOS.in"])

Would you expect that to work with the changes you made to absolute/relative links?

@johannct
Copy link
Member

We probably need to take as common usage to run the doc with the notebooks before proposing a pull request.

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.

3 participants