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

Update infrastructure #4

Merged
merged 19 commits into from
Apr 5, 2023
Merged

Update infrastructure #4

merged 19 commits into from
Apr 5, 2023

Conversation

timmens
Copy link
Member

@timmens timmens commented Apr 3, 2023

What problem do you want to solve?

Match the code base infrastructure with that of estimagic.

Changes

  • Replace testing with tox with pure pytest testing
  • Update main workflow file
  • Add publish to pypi workflow
  • Update issue and pull request templates
  • Update .gitignore
  • Update pre-commit and linting files
  • Update LICENSE
  • Replace RST files with Markdown files (except for documentation files)
  • Delete outdated documentation
  • Use ruff blacklisting (depart from estimagic here)

Todo

  • Target the right branch and pick an appropriate title.
  • Put Closes #XXXX in the first PR comment to auto-close the relevant issue once
    the PR is accepted. This is not applicable if there is no corresponding issue.
  • Update CHANGES.md

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #4 (c66fdc6) into main (f5d91c6) will decrease coverage by 0.17%.
The diff coverage is 93.25%.

@@            Coverage Diff             @@
##             main       #4      +/-   ##
==========================================
- Coverage   98.46%   98.29%   -0.17%     
==========================================
  Files          25       25              
  Lines        1110     1058      -52     
==========================================
- Hits         1093     1040      -53     
- Misses         17       18       +1     
Flag Coverage Δ
unit ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/lcm/create_params.py 100.00% <ø> (ø)
src/lcm/distributions.py 77.77% <0.00%> (ø)
src/lcm/entry_point.py 97.05% <0.00%> (ø)
tests/test_interpolation.py 100.00% <ø> (ø)
src/lcm/example_models.py 84.00% <50.00%> (+0.66%) ⬆️
src/lcm/discrete_emax.py 97.87% <80.00%> (-0.21%) ⬇️
src/lcm/process_model.py 97.84% <87.50%> (-1.11%) ⬇️
src/lcm/dispatchers.py 96.77% <100.00%> (-0.15%) ⬇️
src/lcm/function_evaluator.py 96.05% <100.00%> (-0.06%) ⬇️
src/lcm/grids.py 100.00% <100.00%> (ø)
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@ChristianZimpelmann ChristianZimpelmann left a comment

Choose a reason for hiding this comment

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

I had a look over it and made a few small comments.

Also:

  • Don't you think the documentation could still be of use? It seems to be quite general.. but haven't had a closer look at it.

  • I cannot say much about some parts of the setup (e.g. tox vs pytest, ruff blacklisting)

src/lcm/discrete_emax.py Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
LICENSE Outdated Show resolved Hide resolved
@timmens
Copy link
Member Author

timmens commented Apr 3, 2023

Thank you @ChristianZimpelmann

  • Don't you think the documentation could still be of use? It seems to be quite general.. but haven't had a closer look at it.

Janos recommended deleting it. The docs were comprised mostly of design notes, which are rather developer documentation. I think this developer documentation is very important, but I want to use a different format ---something not on GitHub. Some of Janos' notes would be integrated into this new format.

  • I cannot say much about some parts of the setup (e.g. tox vs pytest, ruff blacklisting)

Maybe @hmgaudecker can comment on this.

@hmgaudecker
Copy link
Member

Happy to do so, but unlikely to happen before our meeting.

@ChristianZimpelmann
Copy link
Member

The docs were comprised mostly of design notes, which are rather developer documentation. I think this developer documentation is very important, but I want to use a different format ---something not on GitHub. Some of Janos' notes would be integrated into this new format.

That makes sense! Just wanted to make sure that the design notes are not lost.

Copy link
Member

@hmgaudecker hmgaudecker left a comment

Choose a reason for hiding this comment

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

Excellent, thanks! I do have many comments, but almost all of them should be very quick and none of them requires any discussion; if you want to leave some of the ruff things as they are, just make a quick note, please.

.envs/testenv.yml Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
src/lcm/distributions.py Show resolved Hide resolved
src/lcm/model_functions.py Outdated Show resolved Hide resolved
@timmens
Copy link
Member Author

timmens commented Apr 5, 2023

Leaving docformatter at v1.5.1 instead of v.1.6.0 because of incompatibilities with black.

@hmgaudecker
Copy link
Member

Leaving docformatter at v1.5.1 instead of v.1.6.0 because of incompatibilities with black.

Let's just delete it, ruff should take care of it eventually.

Copy link
Member

@hmgaudecker hmgaudecker left a comment

Choose a reason for hiding this comment

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

Just for Github to remember I browsed through changes up to this point.

@hmgaudecker
Copy link
Member

(I actually did not mean to approve, else the message would not have made sense, but I might as well have. So just merge as you see fit)

Thanks!

@timmens timmens merged commit ce08e49 into main Apr 5, 2023
@timmens timmens deleted the modernize-infrastructure branch April 5, 2023 11:54
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