MESMER-group / mesmer

spatially-resolved ESM-specific multi-scenario initial-condition ensemble emulator
https://mesmer-emulator.readthedocs.io/en/latest/
GNU General Public License v3.0
22 stars 15 forks source link

MESMER-X Linting #437

Closed veni-vidi-vici-dormivi closed 1 month ago

veni-vidi-vici-dormivi commented 2 months ago

I want to exclude two files from the tests, I guess I should edit the pre commit config in another PR?

mathause commented 2 months ago

It currently fails because there is no mesmer.geospatial module (which is true, the stuff is in mesmer.core.geospatial so it needs import mesmer and we need to call mesmer.spatial.calc_geodist_exact(...). Sorry about that.

I want to exclude two files from the tests, I guess I should edit the pre commit config in another PR?

You want to remove them from running via pytest? I think that should be possible here. But it might be best to get it to run locally, as these import conflicts can be quite annoying...

veni-vidi-vici-dormivi commented 1 month ago

so it needs import mesmer and we need to call mesmer.spatial.calc_geodist_exact(...).

Why can't we use mesmer.core.geospatial.geodist_exact()?

But it might be best to get it to run locally, as these import conflicts can be quite annoying...

So it fails because OLD_train_l_distrib.py and create_emus_l_dristrib.py use functions from a mesmer.utils that does not exist anymore. I talked with Yann about this in #438, and it is because these two are old files which should be replaced by the other/new files. This is why I think it makes sense to exclude them from any testing.

mathause commented 1 month ago

I think the fix now is to move example_mesmer_x.py to the example folder (or to wrap the code in the file to if __name__ == "__main__" block, but the example code should not live in the mesmer code folder).

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 190 lines in your changes are missing coverage. Please review.

Please upload report for BASE (mesmer_x@dce593b). Learn more about missing BASE report.

Files Patch % Lines
mesmer/mesmer_x/OLD_train_l_distrib.py 0.00% 64 Missing :warning:
mesmer/mesmer_x/temporary_config_all.py 0.00% 39 Missing :warning:
mesmer/mesmer_x/train_utils_mesmerx.py 0.00% 28 Missing :warning:
mesmer/mesmer_x/train_l_distrib_mesmerx.py 0.00% 25 Missing :warning:
mesmer/mesmer_x/create_emus_l_distrib.py 0.00% 18 Missing :warning:
mesmer/mesmer_x/temporary_support.py 0.00% 16 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## mesmer_x #437 +/- ## =========================================== Coverage ? 48.12% =========================================== Files ? 51 Lines ? 3254 Branches ? 0 =========================================== Hits ? 1566 Misses ? 1688 Partials ? 0 ``` | [Flag](https://app.codecov.io/gh/MESMER-group/mesmer/pull/437/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MESMER-group) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/MESMER-group/mesmer/pull/437/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MESMER-group) | `48.12% <0.00%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MESMER-group#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mathause commented 1 month ago

Love your commit names ;-) At the risk of making myself very unpopular - I prefer the comments above the code (but ok to leave it like that if it's too stupid for you)

veni-vidi-vici-dormivi commented 1 month ago

I prefer the comments above the code (but ok to leave it like that if it's too stupid for you)

Well that actually makes two of us... I'm switching to a more entertaining Spotify playlist for that one...

mathause commented 1 month ago

Now you are just showing off 😉

veni-vidi-vici-dormivi commented 1 month ago

Okay this is ready imo.

veni-vidi-vici-dormivi commented 1 month ago

Is there no branch protection on this one? I dont see that I need a review...

mathause commented 1 month ago

Probably because you don't want to merge into main. I am looking at the code now.

veni-vidi-vici-dormivi commented 1 month ago

could you have a quick look at the workaround in the example? 🙏 @mathause

mathause commented 1 month ago

Yes ok for me. Could also use # noqa: F841

(needs to be on the line the variable is defined, i.e.

a = (  # noqa: F841
    5,
    6,
)

)

veni-vidi-vici-dormivi commented 1 month ago

Ah I like that a little better thanks!

mathause commented 1 month ago

Close/ reopen to trigger another rtd build

veni-vidi-vici-dormivi commented 1 month ago

It's still failing, but they don't give any information as to why?

veni-vidi-vici-dormivi commented 1 month ago

🎉