VERITAS-Observatory / V2DL3

VERITAS (VEGAS and Eventdisplay) to DL3 Converter
BSD 3-Clause "New" or "Revised" License
8 stars 1 forks source link

Upgrade to GADF standard 0.3 #170

Closed GernotMaier closed 1 year ago

GernotMaier commented 1 year ago

This is an important update to keep updated with the GADF standard. It needs careful review, especially it needs a comparison of the changes implemented with those outlined on the GADF version change page

Changes applied:

Todo:

As field names and values are changes, this is expected to break to testing at this point.

Closes #167

GernotMaier commented 1 year ago

@tobiaskleiner - would be great if you could have a look (take your time). Note that reviewing this PR requires as mentioned above to look at the changes from 0.2 to 0.3 in GADF.

tmcahill commented 1 year ago

Added DATE-AVG and updated other date formats to match the gadf 0.3 spec

Tested and verified new VEGAS date headers by hand. VEGAS CI accounted for only the intended changes.

All looks good to me.

GernotMaier commented 1 year ago

@tmcahill - the vegas test is failing; seems there are still some minor differences in the FITS files. Could you have a look?

tmcahill commented 1 year ago

Hi Gernot--

The differences look intended to me. Please double-check, but I believe that they are only the results of our updates in this pull request. I did not think of a way to have the fitsdiff test fail but notify in the case of intended differences.

A failure in the fitsdiff step of the VEGAS CI should be thought of as a notification of differences rather than a failure. In that case, check the test log for whether the differences are only the intended ones. The VEGAS CI pulls its control inputs from the main branch each time it runs, so there is no need to update the VEGAS CI upon merging a branch with differences.

@deividribeiro I believe you noted this in your e-mail as well, thanks.

GernotMaier commented 1 year ago

Yes, we expect a difference and requires a new default VEGAS file for the comparison. I would prefer to have this fixed before merging this PR, otherwise we miss the whole point of these tests (I’ve updated also the Eventdisplay reference file)

tmcahill commented 1 year ago

@GernotMaier @deividribeiro

After reading the e-mails, I think one of us is misunderstanding something. I designed the VEGAS CI to be fully automated; CI should be wherever possible. The VEGAS CI does not use premade .fits reference files. It pulls the main branch at the time of testing to generate the reference files, then pulls the new branch to generate the test files.

So there are no reference files to update-- the VEGAS CI fitsdiff test will always tell you the differences between the current main branch and your test branch. So if you merge the branch with intended differences, they are automatically incorporated into future CI tests.

Let me know if I am still not understanding the issue.

As a side note, I recommend that ED consider doing this as well. It is not only convenient, but also controls for other potential adverse effects from things like python or package differences (example: the gitlab runner's numpy version may one day update and trim a decimal to a different place than the reference file's, triggering an unimportant difference which may be difficult to track down).

Happy holidays, everyone :)

GernotMaier commented 1 year ago

OK, misunderstood how that CI works and yes, all is good then. Thanks!