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

Mb/explicit #82

Merged
merged 29 commits into from
Jan 25, 2025
Merged

Mb/explicit #82

merged 29 commits into from
Jan 25, 2025

Conversation

sesquideus
Copy link
Contributor

Finished converting the InputSets and Input classes to the implicit detection of tag parameters (detector, target, ...).
Also added more test to cover for new edge cases (not many though).

@sesquideus sesquideus requested a review from chyan26 January 21, 2025 14:05
@chyan26
Copy link
Collaborator

chyan26 commented Jan 23, 2025

I guess there are issues remains with the simulation data. Since there is no conflict with current code. I would suggest we rebase and merge it first.

@sesquideus
Copy link
Contributor Author

Yes, also on my other machine everything works and passes all the tests now... but I do not understand the actual problem.

Still, I think that the benefits of merging now (even without these four tests passing) outweigh the problems it will cause if we diverge too much.

@hugobuddel
Copy link
Contributor

I would prefer that we at least identify the problem before merging anything. Could someone else reproduce the problem?

@sesquideus
Copy link
Contributor Author

My bad.
I did not realize I fixed my pyesorex ("if not" → "is None"). The recipes that are failing are exactly those without parameters.

@hugobuddel
Copy link
Contributor

I've made a bug report, https://jira.eso.org/browse/PIPE-12044 , and two PR's, one with some tests https://gitlab.eso.org/cpl/pyesorex/-/merge_requests/18 , and one with your fix, https://gitlab.eso.org/cpl/pyesorex/-/merge_requests/19 . I don't think you can access those, but I thought it would be good to add a link to them anyway. I can put the branches also on our fork.

But it shouldn't be too hard to work around this bug for now right? Maybe we can automatically add a dummy parameter to every recipe or something like that?

I much prefer adding such an ugly workaround than having broken tests.

@hugobuddel
Copy link
Contributor

Or perhaps we can keep these lines in?

    parameters = cpl.ui.ParameterList([])

I also want to note that I still had problems even with your fixed pyesorex. Because wouldn't removing this line result in parameters to be None? As they are set to None in the base class IIRC. So the is None would still be trigger, just like the if not was, right?

@sesquideus
Copy link
Contributor Author

Yes, we should keep this line in, as a system-wide MetisRecipe default.

If I understand it correctly, cpl.ui.ParameterList([]) is Falsy, and thus it crashes within pyesorex. But it is not None and that works for me now.

@hugobuddel
Copy link
Contributor

By the way, in my bug report I noted that the first recipe on https://www.eso.org/sci/software/pycpl/pycpl-site/user/basics.html does not specify any parameters, so it actually is None there, so that case should be fixed as well (but that is not our problem).

I added a suggestion to add a dummy parameter; would that work for now?

@hugobuddel
Copy link
Contributor

I pushed my test branch to our fork as well, see https://github.com/AstarVienna/pyesorex/tree/addtests .

(By the way, I deleted all the nonsense branches in the pyesorex fork, but they keep turning up again, so need to figure out what is happening.)

@sesquideus
Copy link
Contributor Author

Here we go 🥇

@hugobuddel hugobuddel self-requested a review January 24, 2025 18:55
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.

It would be good to also add these new recipes to the esorex and EDPS tests, but we can do that later.

@sesquideus sesquideus merged commit 9b75085 into main Jan 25, 2025
2 checks passed
@sesquideus sesquideus deleted the mb/explicit branch January 25, 2025 11:14
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