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

Adding updated version of ifu_tellruic recipe #94

Merged
merged 10 commits into from
Feb 6, 2025
Merged

Conversation

wkausch
Copy link
Collaborator

@wkausch wkausch commented Feb 4, 2025

Here's the latest version of the ifu_telluric resipe :-)

@wkausch wkausch requested a review from sesquideus February 4, 2025 11:35
@hugobuddel
Copy link
Contributor

Please check whether the tests pass before assigning a reviewer, or better, run the tests locally through python -m pytest:

metisp/pymetis/src/pymetis/tests/ifu/test_metis_ifu_telluric.py:22: in <module>
    from pymetis.recipes.ifu.metis_ifu_telluric import (MetisIfuTelluric as Recipe, MetisIfuTelluricImpl as Impl)
E     File "/home/runner/work/METIS_Pipeline/METIS_Pipeline/metisp/pymetis/src/pymetis/recipes/ifu/metis_ifu_telluric.py", line 38
E       """Implementation class for metis_ifu_telluric"""
E       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E   SyntaxError: invalid syntax
=========================== short test summary info ============================
ERROR metisp/pymetis/src/pymetis/tests/ifu/test_metis_ifu_telluric.py
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 0.40s ===============================

https://github.com/AstarVienna/METIS_Pipeline/actions/runs/13134572205/job/36646955413

@wkausch
Copy link
Collaborator Author

wkausch commented Feb 4, 2025

Yes, sorry, Actually I didn't want to add ou as reviewer :-)

@wkausch wkausch removed the request for review from sesquideus February 4, 2025 12:00
@hugobuddel
Copy link
Contributor

Yes, sorry, Actually I didn't want to add ou as reviewer :-)

I didn't necessarily intend to actually review, I just got a notification about the PR and thought maybe it would be helpful if you know how to run the tests locally.

@wkausch
Copy link
Collaborator Author

wkausch commented Feb 4, 2025

Yes, that's indeed a good suggestion to make it locally. I'll try to install everything now.

But what I meant is I didn't want to bother you with lots of messages :-). Actually I was surprised that you have been noticed about the crashed tests

@sesquideus
Copy link
Contributor

I went through everything and sequentially eliminated all problems I could find. It gave me an idea how to abstract away a few things in the Product assembly, but that will be left for another PR (after the next delivery).

@wkausch
Copy link
Collaborator Author

wkausch commented Feb 6, 2025

Thanks for eliminating the problems. Does this mean we're done for now?

@sesquideus
Copy link
Contributor

I think it's fine. Moreover, some of the inputs I created here can be reused elsewhere now.

@sesquideus sesquideus requested a review from hugobuddel February 6, 2025 11:54
Copy link
Contributor

@hugobuddel hugobuddel left a comment

Choose a reason for hiding this comment

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

Looks good to me

@sesquideus sesquideus merged commit 513bcc4 into main Feb 6, 2025
2 checks passed
@sesquideus sesquideus deleted the wk_ifu_telluric branch February 6, 2025 11:59
@eiseleb47 eiseleb47 mentioned this pull request Feb 6, 2025
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.

4 participants