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

Multipole: turn on CI test #34

Merged
merged 2 commits into from
Jul 19, 2024
Merged

Multipole: turn on CI test #34

merged 2 commits into from
Jul 19, 2024

Conversation

lwJi
Copy link
Collaborator

@lwJi lwJi commented Jul 19, 2024

No description provided.

@lwJi lwJi requested a review from rhaas80 July 19, 2024 01:35
Copy link
Member

@rhaas80 rhaas80 left a comment

Choose a reason for hiding this comment

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

Why are the data files being updated? Shouldn't the result be the as before? If not then a comment in the commit message would be good explaining why the change is expected.

Copy link
Member

@rhaas80 rhaas80 left a comment

Choose a reason for hiding this comment

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

Are these the number the same as for the Carpet based thorn? The file extensions are different and the commit that introduces them does not really tell anything about whether they have ever been compared to a "known good" version.

@lwJi
Copy link
Collaborator Author

lwJi commented Jul 19, 2024

Why are the data files being updated? Shouldn't the result be the as before? If not then a comment in the commit message would be good explaining why the change is expected.

The change is caused by the bug fix in Z4c

@lwJi
Copy link
Collaborator Author

lwJi commented Jul 19, 2024

Are these the number the same as for the Carpet based thorn? The file extensions are different and the commit that introduces them does not really tell anything about whether they have ever been compared to a "known good" version.

It was not copied from Carpet. I made it up from scratch. I just want have something that tell me I didn't change the code when I commit

@rhaas80
Copy link
Member

rhaas80 commented Jul 19, 2024

ok. For the change in values for Multipole's test it may then be good to add something along the lines of:

Regenerate test data after commit bd461ad "Z4c: fix bug related to the misuse of sym_symm"

so that people can correlate the change in Multipole with the source of the changed values.

Best to give both the git hash and the subject line in case we ever move to a different version control system and the git hashes become meaningless (we moved from svn to git for some repo and thankfully git-svn preserves the svn revision numbers as part of the git commit message, but one would not want to rely on that if possible).

@lwJi lwJi force-pushed the lwji/Multipole branch from d1ca14e to fac4681 Compare July 19, 2024 14:55
@lwJi lwJi force-pushed the lwji/Multipole branch from fac4681 to 70f7d98 Compare July 19, 2024 15:00
@lwJi
Copy link
Collaborator Author

lwJi commented Jul 19, 2024

ok. For the change in values for Multipole's test it may then be good to add something along the lines of:

Regenerate test data after commit bd461ad "Z4c: fix bug related to the misuse of sym_symm"

so that people can correlate the change in Multipole with the source of the changed values.

Best to give both the git hash and the subject line in case we ever move to a different version control system and the git hashes become meaningless (we moved from svn to git for some repo and thankfully git-svn preserves the svn revision numbers as part of the git commit message, but one would not want to rely on that if possible).

Done

@lwJi lwJi merged commit 49a0df9 into main Jul 19, 2024
12 checks passed
@lwJi lwJi deleted the lwji/Multipole branch July 19, 2024 15:44
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.

2 participants