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

Remove RMSE function #3988

Merged
merged 5 commits into from
Apr 12, 2024
Merged

Conversation

kratman
Copy link
Contributor

@kratman kratman commented Apr 10, 2024

Description

Minor removal of code and some cleanup of IDE warnings

Fixes #3982

Type of change

Removes old code

Key checklist:

  • No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@kratman kratman self-assigned this Apr 10, 2024
pybamm/__init__.py Outdated Show resolved Hide resolved
@kratman kratman changed the title Feat/remove rmse Remove RMSE function Apr 10, 2024
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.58%. Comparing base (9fcd56b) to head (62fdd5f).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3988      +/-   ##
===========================================
- Coverage    99.58%   99.58%   -0.01%     
===========================================
  Files          257      257              
  Lines        21252    21242      -10     
===========================================
- Hits         21164    21154      -10     
  Misses          88       88              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

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

All the code removed is fine. Please don't remove those comments. They add structure to the code and make it easier to scan. Removing the empty comment lines (# and nothing else) is fine.

@kratman
Copy link
Contributor Author

kratman commented Apr 10, 2024

All the code removed is fine. Please don't remove those comments. They add structure to the code and make it easier to scan. Removing the empty comment lines (# and nothing else) is fine.

I removed them because they basically all said something along the lines of

# functions from file X
from x import y

Which does not seem to add anything

@valentinsulzer
Copy link
Member

It adds structure and makes the file easier to scan since they are highlighted in different colors by most IDEs

@Saransh-cpp
Copy link
Member

Imports in pybamm's __init__.py file must be in a particular order (autoformatting tools usually mess up this order), so adding a structure with the comments makes sense.

@kratman
Copy link
Contributor Author

kratman commented Apr 11, 2024

It adds structure and makes the file easier to scan since they are highlighted in different colors by most IDEs

To me it did not add anything since it literally said what was right below that in the code, but I will add it back if you insist. In general my changes like this are just trying to clean up stuff that does not add anything like:

#
# Batch Study
#
from .batch_study import BatchStudy

which ends up adding a lot of comments for the exact thing that is below it.

Imports in pybamm's __init__.py file must be in a particular order (autoformatting tools usually mess up this order), so adding a structure with the comments makes sense.

I did not run any auto-formatting here and left the imports in groups. The only thing I did for my IDE was move the constants to the bottom of the file in order to stop the warnings about imports coming after code. The comments did not add any hints about which order things needed to be imported, no warnings about problematic stuff, etc. So I don't think they solve the import order problem either

However, the constants I moved look like they are not actually used and can probably be removed as well. PyBaMM triggers so many warnings in PyCharm that I often cannot see the real problems from the inspections. I try to clean up what I can as I work

@kratman kratman requested a review from valentinsulzer April 12, 2024 14:45
@kratman
Copy link
Contributor Author

kratman commented Apr 12, 2024

Ok everything here is passing now besides Lychee

@valentinsulzer valentinsulzer merged commit ce602d6 into pybamm-team:develop Apr 12, 2024
39 of 40 checks passed
@kratman kratman deleted the feat/removeRMSE branch April 12, 2024 16:11
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.

pybamm.rmse. document return type and args types
3 participants