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

Added csvy parser #928

Merged
merged 41 commits into from
May 29, 2019
Merged

Added csvy parser #928

merged 41 commits into from
May 29, 2019

Conversation

marxwillia
Copy link
Contributor

No description provided.

@wkerzendorf
Copy link
Member

Looks great @marxwillia! We need to use our special yaml parser to make sure it converts quantities strings to quantities - but we can work on this the next few days.

Copy link
Contributor Author

@marxwillia marxwillia left a comment

Choose a reason for hiding this comment

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

Should the user have the flexibility to select a specific density profile and custom abundances? Or is a specific density profile only ever used with uniform abundances?

@marxwillia
Copy link
Contributor Author

Just updated the csvy_model.yml schema file so that it includes the builtin options for density profiles and uniform abundance. I think we want the user to be able to run TARDIS this way in addition to the new flexibility of the csvy model file.

In the density and abundance definitions, the user can choose 'csvy' which has no subfields. My current plan is to make the user responsible for setting up the csv part of the model csvy file correctly, with help from some asserts.

Copy link
Member

@wkerzendorf wkerzendorf left a comment

Choose a reason for hiding this comment

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

run the tests using python setup.py test

tardis/io/schemas/csvy_model.yml Show resolved Hide resolved
tardis/io/schemas/csvy_model.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,454 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

maybe this should go to the documentation?

tardis/io/tests/data/csvy_full.csvy Show resolved Hide resolved
tardis/io/tests/test_csvy_reader.py Outdated Show resolved Hide resolved
tardis/io/tests/test_csvy_reader.py Outdated Show resolved Hide resolved
tardis/io/tests/test_csvy_reader.py Outdated Show resolved Hide resolved
$ref: '#/definitions/abundance/uniform'

required:
- name
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to require a name for the model?

Copy link
Member

Choose a reason for hiding this comment

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

nope - we should not. can the required key live within the node

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure that the required key cannot live within the node.

https://json-schema.org/understanding-json-schema/reference/object.html#required

setup.py Outdated
@@ -113,6 +113,7 @@
package_info['package_data'].setdefault(PACKAGENAME, [])
package_info['package_data'][PACKAGENAME].append('data/*')
package_info['package_data'][PACKAGENAME].append('io/schemas/*')
package_info['package_data'][PACKAGENAME].append('io/tests/data/*')
Copy link
Member

Choose a reason for hiding this comment

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

use this https://github.com/tardis-sn/tardis/blob/master/tardis/io/setup_package.py for adding that.

Copy link
Member

@wkerzendorf wkerzendorf left a comment

Choose a reason for hiding this comment

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

there are three things still open - but then its good to merge.

type: string
description: name of the model being run

description:
Copy link
Member

Choose a reason for hiding this comment

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

are these required keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not currently.

type: string
description: description of the model being run

tardis_model_config_version:
Copy link
Member

Choose a reason for hiding this comment

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

that should be required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tardis_model_config_version is now the only property required.

$ref: '#/definitions/abundance/uniform'

required:
- name
Copy link
Member

Choose a reason for hiding this comment

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

nope - we should not. can the required key live within the node

tardis/io/tests/data/csvy_full.csvy Show resolved Hide resolved
import numpy.testing as npt


DATA_PATH = os.path.join(tardis.__path__[0], 'io', 'tests', 'data')
Copy link
Member

Choose a reason for hiding this comment

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

could be a fixture as well.

@wkerzendorf wkerzendorf merged commit 153736a into tardis-sn:master May 29, 2019
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.

3 participants