Skip to content

Show version information in vp reports#1070

Merged
RosalynLP merged 19 commits into
masterfrom
version_info
Feb 18, 2021
Merged

Show version information in vp reports#1070
RosalynLP merged 19 commits into
masterfrom
version_info

Conversation

@RosalynLP
Copy link
Copy Markdown
Contributor

@RosalynLP RosalynLP commented Jan 25, 2021

Closes #1066.

@RosalynLP
Copy link
Copy Markdown
Contributor Author

RosalynLP commented Jan 25, 2021

To do:

  • Check that all replicas have same version info
  • Return "undefined" for fits without version info
  • Add action to vp-comparefits

Comment thread validphys2/src/validphys/fitdata.py Outdated
version_info = []
# First check if first replica has 'version.info'
p_1 = replica_paths[0] / ('version.info')
if not p_1.is_file():
Copy link
Copy Markdown
Contributor

@siranipour siranipour Jan 25, 2021

Choose a reason for hiding this comment

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

Are you checking if the file exists here? Perhaps p_1.exists() reads a bit more naturally if so

@scarlehoff
Copy link
Copy Markdown
Member

scarlehoff commented Jan 26, 2021

Hi @RosalynLP, instead from the version.info file you should get the information from the .json file. It is somewhat newer and thus older fits (older meaning months) won't have it, but going forward I think all vp actions should get the information from the fit.json file (so that all the others, including fitinfo, can be removed).

Edit: A fit that contains this .json file is NNPDF40_nnlo_as_0118.

@RosalynLP
Copy link
Copy Markdown
Contributor Author

RosalynLP commented Jan 26, 2021

Thanks @scarlehoff, will do - yep I didn't see these before as I was looking at an older fit.

Comment thread validphys2/src/validphys/comparefittemplates/report.md Outdated
@RosalynLP RosalynLP marked this pull request as ready for review January 27, 2021 15:18
@RosalynLP RosalynLP added the run-fit-bot Starts fit bot from a PR. label Jan 27, 2021
@siranipour siranipour added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Jan 27, 2021
@github-actions
Copy link
Copy Markdown

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@github-actions
Copy link
Copy Markdown

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@RosalynLP
Copy link
Copy Markdown
Contributor Author

You can see at the bottom of the report what the version table looks like ... it's OK but personally I think it could be altered a little to look nicer - anyone else have an opinion?

@scarlehoff
Copy link
Copy Markdown
Member

Ideally I would have three columns: name of the program, version of current, version of reference but don't know how difficult it is with vp (and I don't think it is a table we will be look at a lot)

@RosalynLP
Copy link
Copy Markdown
Contributor Author

Ideally I would have three columns: name of the program, version of current, version of reference but don't know how difficult it is with vp (and I don't think it is a table we will be look at a lot)

The reason I didn't do that is because I also wanted the option to just return the info for one fit only. But I could do two actions, one for just one fit and one (to go in the vp report) with both fits combined.

@scarlehoff
Copy link
Copy Markdown
Member

Ah, makes sense. For me it's ok like this (the only thing I would remove is the index number from the first column I guess).

@RosalynLP
Copy link
Copy Markdown
Contributor Author

Yep I agree it's ugly and unnecessary

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Jan 28, 2021

Ideally I would have three columns: name of the program, version of current, version of reference but don't know how difficult it is with vp (and I don't think it is a table we will be look at a lot)

Very easy. Have a look at how collect works (as applied to e.g. the fit summary among many things).

@RosalynLP
Copy link
Copy Markdown
Contributor Author

Ideally I would have three columns: name of the program, version of current, version of reference but don't know how difficult it is with vp (and I don't think it is a table we will be look at a lot)

Very easy. Have a look at how collect works (as applied to e.g. the fit summary among many things).

Yes my plan was to do one joint table and one separate table so there are options

Comment thread validphys2/src/validphys/fitdata.py Outdated
@RosalynLP
Copy link
Copy Markdown
Contributor Author

RosalynLP commented Jan 29, 2021

OK I think the output is much nicer now:

image

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Jan 29, 2021

As an aside, please delete all "4.0" fits from everywhere. And consider burning the computers where they were installed as an additional safety measure. CC @scarlehoff .

@scarlehoff
Copy link
Copy Markdown
Member

No problem

Burn!!!

wrt to the current output for the versions, I like it.

@RosalynLP
Copy link
Copy Markdown
Contributor Author

As an aside, please delete all "4.0" fits from everywhere. And consider burning the computers where they were installed as an additional safety measure. CC @scarlehoff .

yeah don't worry I just installed them for this because they had version information

Comment thread validphys2/src/validphys/fitdata.py Outdated
Comment thread validphys2/src/validphys/fitdata.py Outdated
Comment thread validphys2/src/validphys/fitdata.py Outdated
Comment thread validphys2/src/validphys/fitdata.py Outdated
Comment thread validphys2/src/validphys/fitdata.py Outdated
RosalynLP and others added 3 commits February 16, 2021 15:59
Co-authored-by: siranipour <43517072+siranipour@users.noreply.github.com>
Co-authored-by: Cameron Voisey <32741139+voisey@users.noreply.github.com>
@RosalynLP
Copy link
Copy Markdown
Contributor Author

The checks pass now so can we merge this at the code call today?

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Feb 17, 2021

I thought this was merged already. @voisey @siranipour please approce and merge asap.

Comment thread validphys2/src/validphys/fitdata.py Outdated
Comment thread validphys2/src/validphys/fitdata.py Outdated
Comment thread validphys2/src/validphys/fitdata.py
Co-authored-by: Cameron Voisey <32741139+voisey@users.noreply.github.com>
Comment thread validphys2/src/validphys/fitdata.py
Comment thread validphys2/src/validphys/fitdata.py Outdated
Comment thread validphys2/src/validphys/fitdata.py Outdated
Co-authored-by: siranipour <43517072+siranipour@users.noreply.github.com>
@RosalynLP
Copy link
Copy Markdown
Contributor Author

Shall I merge?

@siranipour
Copy link
Copy Markdown
Contributor

Yep good to go!!

@RosalynLP RosalynLP merged commit 5439f8e into master Feb 18, 2021
@RosalynLP RosalynLP deleted the version_info branch February 18, 2021 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-fit-bot Starts fit bot from a PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show version information more prominently

5 participants