-
Notifications
You must be signed in to change notification settings - Fork 1
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
Started filling out .janno for Fischer_Gauls #24
Started filling out .janno for Fischer_Gauls #24
Conversation
Hi @Kavlahkaff, some quick replies:
Note that you can mark a PR as "draft" and then make it "ready for review" later to indicate that it's finalised from your side for now. Let us know whether it's finalised, then we can take a deeper look. |
Hi @Kavlahkaff, great. Some quick notes:
Did you have a chance to meet with @nevrome to get a primer on how to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The janno seems to have a line of whitespace at the end.
Beyond that, the updated janno has an additional column named Column1
. I think the column might be empty all the way down?
As mentioned in #22, please do not overwrite any of the processing statistics produced by MINOTUAR. The RateX/Y columns in particular look off here, as they have values in E+13~E+14. The expected values for these are ~0-1.5
Ok, I realised the issue was that the original janno file from MINOTAUR had been overwritten in the original PR. I reverted that change and pulled the changes into this PR too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, now the git changes look way cleaner.
Please:
- revert the changes to the already-filled columns (
Rate*X/Y
,Contamination*
etc) - Remove
Column1
, but first check that the column is indeed empty and does not cause any column-wise shifts in the metadata.
Hi @stschiff, @TCLamnidis, I have made the changes you requested and ran trident validate with no errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
janno needs new line at EOF, but otherwise looks great! Thanks!
f677e9d
into
poseidon-framework:2022_Fischer_Gauls
Started filling out the .janno.
These are some columns where I was unsure: