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

Qa excl #168

Merged
merged 9 commits into from
May 26, 2020
Merged

Qa excl #168

merged 9 commits into from
May 26, 2020

Conversation

MRossol
Copy link
Collaborator

@MRossol MRossol commented May 22, 2020

Add heatmap plots of final exclusions mask

Full pipeline test is in progress

@MRossol MRossol added the feature New feature or request label May 22, 2020
@MRossol MRossol requested a review from grantbuster May 22, 2020 22:09
@MRossol
Copy link
Collaborator Author

MRossol commented May 22, 2020

Full test ran, no wonder if died, the .npy file is 3.1 GB!

grantbuster
grantbuster previously approved these changes May 26, 2020
Copy link
Member

@grantbuster grantbuster left a comment

Choose a reason for hiding this comment

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

Nice! Looks good. If you have time, consider re-organizing the QAQC and Summarize classes to be more explicit on which file types they are handling. Theyre purposed for h5 files right now (gen, econ outputs) and have "hacks" to summarize supply curve and exclusions. Would be better to have abstract classes and concrete for applications to "normal" h5, exclusions, and sc. For example: QaQC.exclusions_mask(cls), this doesn't even use the cls arg? Seems like trying to fit a square peg in a round hole.

Update QaQc, cli, tests, and docs
@MRossol
Copy link
Collaborator Author

MRossol commented May 26, 2020

Okay, @grantbuster let me know what you think. I heavily refactored summary, but only lightly changed QaQc. I kept just one QaQc class but reduced in init to just be out_dir, so its mainly just a set of staticmethods and classmethods to run QaQc on h5, sc, or excl

@grantbuster
Copy link
Member

@MRossol, looks great. I agree its maybe a bit cumbersome with the multiple classes but the heirarchy is so much more obvious.

Any reason to run the test on Eagle again? Can you update the example pipeline config? Maybe add in the exclusion plot interval kwarg?

@MRossol
Copy link
Collaborator Author

MRossol commented May 26, 2020

I just re-ran the cli test and no issues. The only real excl kwarg would be point_step... I can change that to 200 or 1000 for the test?

@grantbuster
Copy link
Member

@MRossol, yeah i more just think its nice to have in the example so anyone copying the example config knows its an options.

Can you add the exclusion block to the example file though? I just made a smaller standalone config for just the exclusions on Eagle and it never made its way to this file: https://github.com/NREL/reV/blob/qa_excl/examples/full_pipeline_execution/config_qa-qc.json

@MRossol
Copy link
Collaborator Author

MRossol commented May 26, 2020

I just came to the same conclusion when I went back to look at the example config. Just added the excl module to the example config from your eagle file

@grantbuster grantbuster merged commit 5b7a821 into master May 26, 2020
@grantbuster grantbuster deleted the qa_excl branch May 26, 2020 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants