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.

Sign up for GitHub

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

Jump to bottom

Adding Config Value Validation for Different Parameters via Config File #1554

Merged
merged 6 commits into from
May 4, 2021
Merged

Adding Config Value Validation for Different Parameters via Config File #1554

merged 6 commits into from
May 4, 2021

Conversation

DhruvSondhi
Copy link
Contributor

This PR aims to implement config validation for values that are passed through the .yml file.
This Implements #1343

Description
This PR aims to implement validation for values that can be passed through the tardis configuration file.
These are aimed at Density & Abundances in the current iteration.

Motivation and context
The tardis configuration file allows for non-realistic values to be passed without raising any errors. This PR aims to fix this & implement validation for the values so as to allow only real physics values to be added

How has this been tested?

  • Testing pipeline.
  • Other.

Examples
None at current status :)

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I will update the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.

@DhruvSondhi
Copy link
Contributor Author

DhruvSondhi commented Apr 26, 2021 •
edited
Loading

I will be adding the validation for the Density as well ... But I have a query @Rodot-, Do we need to validate other parameters or other ways of parsing data such as the csvy file, abundance files?

@DhruvSondhi DhruvSondhi force-pushed the config_parser_validation branch from 34fe02c to 6c6fc89 Compare April 26, 2021 14:08
@codecov
Copy link

codecov bot commented Apr 26, 2021 •
edited
Loading

Codecov Report

Merging #1554 (1cb311b) into master (923e59d) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1554      +/-   ##
==========================================
+ Coverage   68.18%   68.24%   +0.05%     
==========================================
  Files          73       73              
  Lines        6308     6373      +65     
==========================================
+ Hits         4301     4349      +48     
- Misses       2007     2024      +17
Impacted Files Coverage Δ
tardis/tardis/io/config_reader.py 82.35% <0.00%> (-3.98%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 923e59d...1cb311b. Read the comment docs.

@andrewfullard
Copy link
Contributor

These tests should be part of the JSON Schema, not if statements in code. See e.g. https://json-schema.org/understanding-json-schema/reference/numeric.html

The schema are located in /tardis/io/schemas

@DhruvSondhi DhruvSondhi force-pushed the config_parser_validation branch 3 times, most recently from 6e78ebb to e87a51f Compare April 27, 2021 07:47
andrewfullard
andrewfullard reviewed Apr 27, 2021
View reviewed changes
Copy link
Contributor

@andrewfullard andrewfullard left a comment

Choose a reason for hiding this comment

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

Looks good so far. I think @Rodot- can provide more parameters that should have additional validation.

@Rodot-
Copy link
Contributor

Rodot- commented Apr 27, 2021

A couple I can think of: Any physical quantity should be positive (temperatures, velocities, times, densities, etc). time_explosion can't be zero. For various density profiles, make sure that the start velocity is lower than the stop velocity.

@DhruvSondhi
Copy link
Contributor Author

DhruvSondhi commented Apr 27, 2021 •
edited
Loading

Hello, @andrewfullard @Rodot- ... While going through the code for validating the different parameters in the YAML file, I think changes are to be made to the config_reader.py & config_validator.py files (I am not absolutely sure about the second one but could be dependent) for checking the valid conditions ... This is due to the fact the there is an astropy quantity based object being used as the parameter. Support for this is not present in the normal JSON Schema & hence, we need to make changes in alternative ways. What do you suggest about the same? 🤔

@DhruvSondhi
Copy link
Contributor Author

Another thing that is troubling me, allowed_value & allowed_type are not being validated in the schema... I have tried many combinations for the same but the schema validation just ignores this property... if these properties work as expected then altogether the above approach can be removed 🤔

@DhruvSondhi DhruvSondhi force-pushed the config_parser_validation branch 5 times, most recently from f7fd1d9 to a46c2cd Compare April 28, 2021 08:53
@DhruvSondhi DhruvSondhi force-pushed the config_parser_validation branch 4 times, most recently from a8af420 to af08045 Compare April 28, 2021 10:53
@Rodot-
Copy link
Contributor

Rodot- commented Apr 29, 2021

Could you add some tests where you make sure that it fails on invalid config files in all of the places you added checks for?

@DhruvSondhi DhruvSondhi force-pushed the config_parser_validation branch from 2cc7e0b to e8622bd Compare April 30, 2021 16:39
@DhruvSondhi DhruvSondhi force-pushed the config_parser_validation branch from e8622bd to 22ba5a4 Compare April 30, 2021 20:44
@DhruvSondhi DhruvSondhi force-pushed the config_parser_validation branch from 9ff0082 to ac10a4f Compare May 1, 2021 07:01
@DhruvSondhi DhruvSondhi force-pushed the config_parser_validation branch from 949bb07 to 3832d91 Compare May 1, 2021 07:03
@DhruvSondhi
Copy link
Contributor Author

DhruvSondhi commented May 3, 2021 •
edited
Loading

Hello @Rodot-, I have added the tests for all the required sections that are being validated in the tardis_config YAML file. Please do let me know if further changes or additions are required :)

@Rodot-
Copy link
Contributor

Rodot- commented May 3, 2021

@DhruvSondhi This is looking good, we just need some docstrings added to the tests then we should be good to merge

@DhruvSondhi
Copy link
Contributor Author

@Rodot- @andrewfullard Should I squash all the commits?

@DhruvSondhi DhruvSondhi force-pushed the config_parser_validation branch from c5e7d2f to 1cb311b Compare May 3, 2021 14:53
@andrewfullard andrewfullard merged commit c384d74 into tardis-sn:master May 4, 2021
@DhruvSondhi DhruvSondhi deleted the config_parser_validation branch May 5, 2021 05:52
@DhruvSondhi DhruvSondhi mentioned this pull request May 28, 2021
Open
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
@DhruvSondhi
Adding Config Value Validation for Different Parameters via Config Fi…
0ebad3a 
…le (tardis-sn#1554)

* Adding Config Validation for Uniform Abundance via Schema

* Adding Validation for time_explosion

* Validating based on different sections

* Fixing CSVY Tests Implementation

* Added Configuration Validation Tests

* Added Docstrings for the validation tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewfullard andrewfullard andrewfullard approved these changes

@Rodot- Rodot- Rodot- approved these changes

Assignees
No one assigned
Labels
configuration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DhruvSondhi @andrewfullard @Rodot-