Skip to content

[WIP]: Python commondata parser#769

Merged
scarrazza merged 61 commits into
masterfrom
python-commondata-parser
Jun 24, 2020
Merged

[WIP]: Python commondata parser#769
scarrazza merged 61 commits into
masterfrom
python-commondata-parser

Conversation

@RosalynLP
Copy link
Copy Markdown
Contributor

Addresses the first part of #379. In a similar vein to #404 we want to load in the commondata from file and parse it, converting to central values, systematics etc. This is to be done using pandas.

@RosalynLP RosalynLP self-assigned this May 8, 2020
@Zaharid Zaharid requested a review from scarrazza May 11, 2020 10:48
@RosalynLP RosalynLP marked this pull request as draft May 13, 2020 10:19
@siranipour siranipour self-requested a review May 18, 2020 08:16
@RosalynLP
Copy link
Copy Markdown
Contributor Author

RosalynLP commented May 20, 2020

So far I've implemented most of this, i.e.

  • Created CommonData class
  • parse_commondata parses a file to a CommonData
  • load_commondata takes a CommonDataSpec and produces a CommonData using parse_commondata
  • made test function in test_commondataparser.py

Still to do:

  • Populate the CommonDataInfo object I created - do we actually need this though? I am not sure if it's that useful
  • Error messages, e.g. raise a BadCommonDataError if parsing can't be done
  • Similarly parse the systypes files
  • Allow for >3 types of kinematics

@siranipour siranipour force-pushed the python-commondata-parser branch from d774074 to 3a08e67 Compare June 6, 2020 12:43
@siranipour siranipour force-pushed the python-commondata-parser branch 3 times, most recently from dba0f95 to 1cc8329 Compare June 8, 2020 10:36
@siranipour
Copy link
Copy Markdown
Contributor

There is a problem at the moment that the commondata files start indexing from 1, while cuts objects start indexing from 0. This makes things a bit of a mess, so what's the verdict on indexing the dataframes from 0?

@RosalynLP
Copy link
Copy Markdown
Contributor Author

There is a problem at the moment that the commondata files start indexing from 1, while cuts objects start indexing from 0. This makes things a bit of a mess, so what's the verdict on indexing the dataframes from 0?

Although I like the idea of indexing from 0 I worry that if someone were to identify point 224 for example, and then wanted to find that entry in the commondata file for whatever reason we'd want those two to match up. I'd think shifting the cuts would be best in this scenario.

@siranipour
Copy link
Copy Markdown
Contributor

Yeah I worry both will probably be a bit of a mess, but your method less so. I'll do that in the next commit!

@RosalynLP
Copy link
Copy Markdown
Contributor Author

Thanks @siranipour

@siranipour siranipour force-pushed the python-commondata-parser branch from 61f737f to 2325c4c Compare June 8, 2020 13:31
Comment on lines +185 to +186
Note if the first data point passes cuts, the first entry
of ``cuts`` should be ``0``.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is rather unclear. It reads to me like it should be a list starting with zero.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That is what we mean right? If datapoint 1 passed cuts then cuts = [0, ...

Comment thread validphys2/src/validphys/coredata.py Outdated
siranipour added 4 commits June 8, 2020 15:20
We must do this as cuts indexing starts at 0 while commondata
indexing starts at 1.
@siranipour siranipour force-pushed the python-commondata-parser branch from 2325c4c to 11c4b5d Compare June 8, 2020 14:21
@siranipour
Copy link
Copy Markdown
Contributor

What else is left for this PR?

@scarrazza scarrazza merged commit 25056e9 into master Jun 24, 2020
@scarrazza scarrazza deleted the python-commondata-parser branch June 24, 2020 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants