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

ci: add flake8 and fix warnings #79

Merged
merged 1 commit into from
Nov 5, 2020
Merged

ci: add flake8 and fix warnings #79

merged 1 commit into from
Nov 5, 2020

Conversation

jenhagg
Copy link
Collaborator

@jenhagg jenhagg commented Nov 4, 2020

Purpose

Add flake8 linting as done in powersimdata and refactor the github action yaml to use tox. Fixes include defining an undefined variable and removing unused imports.

What it does

  • tox.ini - configure tox for local use or ci
  • .yml files - added github action for pytest and renamed linting workflow
  • add pytest to requirements file

@jenhagg jenhagg requested review from ahurli and danielolsen November 4, 2020 20:06
@jenhagg jenhagg self-assigned this Nov 4, 2020
@jenhagg jenhagg added this to the VOTE milestone Nov 4, 2020
@@ -74,7 +71,7 @@ def extract_data(results):
outputs = {}

tic = time.process_time()
for filename in tqdm(results):
for i, filename in tqdm(enumerate(results)):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't have an easy way to test this but I believe we reference this here (currently L88, below).

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you're right. This is an artifact of our previous load shedding strategy: on any infeasible interval, shed 5% of the original demand for that interval and re-run (as many times as necessary), so that a string of interval_num:shed_amount_... is enough to specify all load shedding details.

Breakthrough-Energy/PowerSimData#219 is related to the need to improve the summarization of load shed results in the new more-flexible load shedding method.

{format,checkformatting}: black
{format,checkformatting}: isort
flake8: flake8
changedir = pyreisejl
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of this file was copied from powersimdata but I added the changedir option so passing the current directory to commands still works. Also, the flake8 ignore section is customized for this repo.

@danielolsen
Copy link
Contributor

Why do we change version numbers in requirements.txt and add pytest?

@jenhagg
Copy link
Collaborator Author

jenhagg commented Nov 4, 2020

Why do we change version numbers in requirements.txt and add pytest?

Added pytest so when tox sets up the environment to run tests, it can simply install from requirements.txt instead of having pytest as a separate dependency.

Changed versions to compatible release, which would be the same as adding a <max_version constraint to the existing file. I think the actual numbers used are the current latest versions, so should be consistent with the >=version spec we have now.

@danielolsen
Copy link
Contributor

Changed versions to compatible release

Do we know for sure that these new version are compatible with everything that we're doing on the python side? Our test coverage is pretty poor at this point, we're not actually testing to ensure that we can read and convert .mat files in the way that REISE.jl is currently producing them.

@jenhagg
Copy link
Collaborator Author

jenhagg commented Nov 4, 2020

I can't say much about it unless we've tested with what is checked into develop now, but can say the changes in this PR won't update anything beyond that (it's actually more restrictive, only allowing patch updates but not major or minor versions).

@danielolsen
Copy link
Contributor

danielolsen commented Nov 4, 2020

@jon-hagg I see what you mean now, this way is indeed cleaner.

To improve the test coverage here, I think the ultimate thing to do is to:

  • Use Julia (via python) to write a) the input.mat file, and b) a set of result_*.mat files in the way that REISE.jl writes them now.
  • Test that we can adequately interpret and convert the matfile v7.3 input.mat file and save to a scipy-compatible v7 *_grid.mat file.
  • Test that we can adequately interpret and process the result_*.mat files into .pkl files (make sure that we get the standard files plus make sure that we get additional files if storage and/or load shedding are present)

@jenhagg
Copy link
Collaborator Author

jenhagg commented Nov 4, 2020

Agreed, I think that's really the only way to validate compatibility for sure. Semantic versioning is good but not quite enough. Side note, we could probably run the existing julia tests in github as well (something to look into when there is time)


[testenv]
deps =
pytest: -rrequirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

typo here? rrequirements -> requirements? Should this typo create an error in the CI check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is actually right (otherwise I'd expect pytest to fail) - docs here

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoa! TIL.

Copy link
Contributor

@danielolsen danielolsen left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@jenhagg jenhagg merged commit 4ec79e1 into develop Nov 5, 2020
@jenhagg jenhagg deleted the jon/api branch November 5, 2020 23:53
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.

2 participants