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

Jet Stirred Reactor (JSR) #126

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Jet Stirred Reactor (JSR) #126

wants to merge 22 commits into from

Conversation

skrsna
Copy link

@skrsna skrsna commented Jun 21, 2019

Not yet ready for review.

  • Fixes #
  • Tests added
  • Added entry into CHANGELOG.md
  • Documentation updated

Changes proposed in this pull request:

  • Adding the ability to represent Jet Stirred Reactors (JSR) experiments

@pr-omethe-us/chemked

@skrsna
Copy link
Author

skrsna commented Jun 21, 2019

The schemas etc are still severely broken, but if @kyleniemeyer and @bryanwweber would like to look at the pyked/tests/testfile_jsr1.yaml file, it should be ready for comments now.

@rwest
Copy link
Member

rwest commented Jul 2, 2019

The example JSR yaml needs the CSV file to go alongside it

@rwest
Copy link
Member

rwest commented Jul 5, 2019

We made some progress. A bit of refactoring, so that DataPoint is now a superclass and IgnitionDataPoint and SpeciesProfileDataPoint are subclasses. The latter can read data from a CSV file. We think.

@kyleniemeyer
Copy link
Member

one quick comment: should the csv file have a # or something at the beginning of the header line?

pyked/chemked.py Outdated
if units: break
#todo: schema should enforce at most 1 units entry

value_properties = [ f'{value} {units}' ]
Copy link
Member

Choose a reason for hiding this comment

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

no f-strings in python 3.5, which travis includes?

@skrsna
Copy link
Author

skrsna commented Jul 5, 2019

I updated process_column in DataPoint superclass and SpeciesProfileDataPoint subclass to read inlet, outlet composition, and temperature from the csv file. The code is messy for now but it's a start. Have a nice weekend y'all!

@mefuller
Copy link
Collaborator

@sevyharris Should I start looking to review this PR or should it be considered in progress?
They're 2.5 years old, but there's an empty set of check boxes at the top of the PR

@sevyharris
Copy link

Hi @mefuller, it's still in progress, but I would love for you to review it when it's ready, hopefully in a day or so!

@mefuller
Copy link
Collaborator

@sevyharris @rwest great to hear - I am definitely interested in reviving this project and would be happy to see PRs from your group (especially anything related to the issues with cerberus)

@bryanwweber
Copy link
Member

@mefuller @sevyharris although I don't have a particular interest in making any final decisions, I'd suggest replacing Cerberus with another library for the data validation. There's been a ton of innovation in this space in the last six years, especially with type checking becoming a true part of the language. I don't have any specific suggestions, but perhaps something like Pydantic would work here (although that's primarily built for web validation).

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.

7 participants