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

Naming and rewrite the very old ones #38

Merged
merged 26 commits into from
Jun 1, 2023
Merged

Naming and rewrite the very old ones #38

merged 26 commits into from
Jun 1, 2023

Conversation

MaceKuailv
Copy link
Owner

No description provided.

@MaceKuailv MaceKuailv temporarily deployed to pypi May 30, 2023 01:10 — with GitHub Actions Inactive
@MaceKuailv
Copy link
Owner Author

issue #36 is taken care of.
There is also some improvement on naming (issue #33).
itertools.combinations are used instead of home made combination (issue #35 )
This version should pass all tests, but zenodo is done...
image

@MaceKuailv MaceKuailv temporarily deployed to pypi May 30, 2023 02:14 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@9205d40). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #38   +/-   ##
=======================================
  Coverage        ?   93.15%           
=======================================
  Files           ?       11           
  Lines           ?     2587           
  Branches        ?      705           
=======================================
  Hits            ?     2410           
  Misses          ?       75           
  Partials        ?      102           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@MaceKuailv MaceKuailv temporarily deployed to pypi May 30, 2023 02:40 — with GitHub Actions Inactive
@malmans2
Copy link
Contributor

Nice!
pylint is another good tool to enforce code standards.
I suggest to run it on your code.
It's a very strict tool, so you shouldn't necessarily try to address all the issues reported by pylint.

@MaceKuailv MaceKuailv temporarily deployed to pypi May 30, 2023 17:29 — with GitHub Actions Inactive
@MaceKuailv MaceKuailv temporarily deployed to pypi May 30, 2023 19:24 — with GitHub Actions Inactive
@MaceKuailv
Copy link
Owner Author

All global and eval eliminated, which means issue #16 resolved

@MaceKuailv MaceKuailv temporarily deployed to pypi May 31, 2023 01:33 — with GitHub Actions Inactive
@MaceKuailv
Copy link
Owner Author

Used an attributeable_dictionary object to refactor many grid operations. Also added a place to include documentation for what I mean by rel coordinate.
Consider issue #35 resolved.

@malmans2
Copy link
Contributor

A couple of thoughts:

  1. Are you sure you need the AttributableDict? At first glance it seems a bit over-complicated. Maybe attrs can do the job, or you can just simplify the code? attrs is a very robust package, so if it's helpful you can have it as a mandatory dependency (it's possible that one of your dependencies already uses it).
  2. Looks like you use "dunder" methods in various places. You shoud be very careful, and only use those when you really need it. An example: to get the length of a list, do len([1]) rather than [1].__len__()
  3. Instantiated classes should be snake_case and not CamelCase. e.g.: ds = xr.Dataset()

@MaceKuailv
Copy link
Owner Author

MaceKuailv commented May 31, 2023

For 1, I do need the update method of dictionary to make it more readable.
For 2, I will refactor the __dict__ today with vars(). However, I don't know what to do with the setattr methods.
For 3, variables like HRel are actually subclasses.

@MaceKuailv MaceKuailv temporarily deployed to pypi May 31, 2023 21:30 — with GitHub Actions Inactive
@MaceKuailv
Copy link
Owner Author

I am happy with the style of the convention. consider issue #34 resolved.

@MaceKuailv MaceKuailv temporarily deployed to pypi May 31, 2023 23:36 — with GitHub Actions Inactive
@MaceKuailv MaceKuailv temporarily deployed to pypi June 1, 2023 01:45 — with GitHub Actions Inactive
@MaceKuailv MaceKuailv temporarily deployed to pypi June 1, 2023 01:56 — with GitHub Actions Inactive
@MaceKuailv
Copy link
Owner Author

I am relatively happy with the current naming convention and readability in general. For phase 1, I consider issue #33 and finally issue #38 resolved. Also, it has become too big a branch.

@malmans2 Comments?

@malmans2
Copy link
Contributor

malmans2 commented Jun 1, 2023

Looks good!

@malmans2
Copy link
Contributor

malmans2 commented Jun 1, 2023

A couple of minor things I noticed:

  1. pandas is a mandatory dependency of xarray, so you don't need the try/except + get rid of the "hidden" import as _pd
  2. do not raise bare Exceptions, they are very hard to catch. The most common is ValueError, but take a look at all exceptions available if you think that there's something better for you use. Alternatively you can create your own exception, but I would only do that if you are going to catch it somewhere. E.g.:
class SeaDuckError(Exception):
    ...
    pass

@MaceKuailv
Copy link
Owner Author

I think I did get rid of the Exceptions, except for one of them in a compiled function. except IndexError simply does not work. There should be a workaround. The wifi is down today. I will merge this first and work on the issues you mentioned from a new branch.

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