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

Legend bug in wavetilts #1896

Open
wants to merge 2 commits into
base: release
Choose a base branch
from
Open

Legend bug in wavetilts #1896

wants to merge 2 commits into from

Conversation

kbwestfall
Copy link
Collaborator

As reported via Slack.

@kbwestfall kbwestfall requested review from profxj, debora-pe and rcooke-ast and removed request for profxj February 5, 2025 21:08
Copy link
Collaborator

@profxj profxj left a comment

Choose a reason for hiding this comment

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

thx!

you might want to direct to release

Copy link
Collaborator

@rcooke-ast rcooke-ast left a comment

Choose a reason for hiding this comment

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

Thanks! Is it worthwhile adding something to the CHANGES log?

@kbwestfall kbwestfall changed the base branch from develop to release February 6, 2025 18:53
@kbwestfall
Copy link
Collaborator Author

you might want to direct to release

Good idea and done! Note that PRs to release now always trigger a new patch release. So there's more work involved in hotfixes, but it's something we need to get in the habit of doing.

Thanks! Is it worthwhile adding something to the CHANGES log?

Yes, and done!

I need to run the dev-suite before merging because this will trigger a new release.

Copy link
Collaborator

@debora-pe debora-pe left a comment

Choose a reason for hiding this comment

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

Thanks @kbwestfall

@kbwestfall
Copy link
Collaborator Author

I'm getting unrelated test failures:

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  257 passed, 6579 warnings in 693.23s (0:11:33) ---
--- PYTEST UNIT TESTS PASSED  152 passed, 136953 warnings in 1249.30s (0:20:49) ---
--- PYTEST VET TESTS FAILED  2 failed, 61 passed, 106683 warnings in 7597.49s (2:06:37) ---
--- PYPEIT DEVELOPMENT SUITE FAILED 2/248 TESTS  ---
Failed tests:
    gtc_osiris/R2500R pypeit
    keck_hires/J0100+2802_H204Hr_RED_C1_ECH_0.75_XD_1.69_1x2 pypeit
Skipped tests:
Testing Started at 2025-02-06T19:52:16.692758
Testing Completed at 2025-02-07T17:39:53.816517
Total Time: 21:47:37.123759

I don't understand the gtc_osiris/R2500R pypeit failure; it seems like the test wasn't even executed...

The recorded error for the keck_hires failure is:

 res = np.fmax.reduce(a, axis=axis, out=out, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: zero-size array to reduction operation fmax which has no identity

Has anyone seen this before? Is it fixed in the develop branch?

The two vet test failures are:

FAILED ../PypeIt-development-suite/vet_tests/test_datacube.py::test_coadd_datacube - AssertionError:
 assert np.float64(1.7027376180641511) < (0.1 * np.float64(3...
FAILED ../PypeIt-development-suite/vet_tests/test_skysub.py::test_skysub - AssertionError: wavelengt
h should be the same

We continue to run into the first one. For the 2nd one in test_skysub.py the dev-suite develop branch uses np.allclose instead of np.array_equal, which allowed the test to pass when I saw this failure earlier.

Given all these are failures that should also occur in the release branch, I'm tempted to simply merge and release the new tag. Thoughts?

@rcooke-ast
Copy link
Collaborator

I'm fine with merging 👍

I'll be coming back to the datacube test failure at some point to understand why that keeps cropping up. The test_skysub failure is currently failing on the develop branch as well.

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