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

LAMA to Dask: TODO tag cataloguing & further tidy #437

Conversation

sadielbartholomew
Copy link
Member

@sadielbartholomew sadielbartholomew commented Aug 15, 2022

Tidies the lama-to-dask branch further in the following ways:

  • cataloguing the TODODASK markers left over from development work towards Replace LAMA with Dask to parallelise cf.Data #182 to aid with LAMA to Dask: address TODODASK items #433, because some are related and also some trivial, so it is very useful going forward to group and distinguish these (without stopping TODODASK overall from being searchable), notably to use:
    • TODODASKVER for cases of placeholders for the final version for the release with the Dask migration which can now be sorted easily in a targeted way via a single sed or similar command;
    • TODODASKDOCS for required docstring or documentation updates;
    • TODODASKMSG for use-facing messages e.g. error messages;
    • TODODASKAPI for API changes or considerations (potentially to bump to the release after Dask as per recent versioning discussions);
    • TODODASKDEPR for anything deprecation related;
    • anything else presently left as-is with TODODASK (miscellaneous etc.), these probably being the most meatly/difficult TODOs of all.
  • converting any usage of np.ma.masked to cf.masked in the current docstring examples for consistency;
  • address a TODO not related to the Dask migration;
  • fix a typo.

See individual commits for separate work on each of the above.

@sadielbartholomew sadielbartholomew added code tidy dask Relating to the use of Dask labels Aug 15, 2022
@sadielbartholomew sadielbartholomew self-assigned this Aug 15, 2022
@sadielbartholomew sadielbartholomew marked this pull request as draft August 15, 2022 23:05
Specifically, applied command:
    find . -type f | xargs sed -i 's/\.\. versionadded::
    TODODASK/\.\. versionadded:: TODODASKVER/g'
Specifically, applied command:
    find . -type f | xargs sed -i 's/version TODODASK/version
    TODODASKVER/g'
Targets 'version="TODODASK"' w/ command form stated in prev. commits.
Copy link
Collaborator

@davidhassell davidhassell left a comment

Choose a reason for hiding this comment

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

Thanks, Sadie. This'll make things easier, and good spot on the Exception -> ValueError item.

@sadielbartholomew
Copy link
Member Author

Thanks for reviewing David. I am about to push a few more 'TODO' tag identifier updates (sorry I should have mentioned that it might be best to review at the end, keeping this in draft form) but they are all quite trivial as with the rest of this PR, so if you don't think it necessary there's no need to review the few extra commits I'll push in a moment.

@davidhassell
Copy link
Collaborator

Ah - sorry for being slightly precipitous. Good idea to merge whenever you're ready.

@sadielbartholomew
Copy link
Member Author

sadielbartholomew commented Aug 16, 2022

Ah - sorry for being slightly precipitous.

My fault in fact, sorry, I opened this in non-draft form by mistake so you were notified pre-maturely. Should have commented as such.

Good idea to merge whenever you're ready.

Thanks, will do.

@sadielbartholomew sadielbartholomew marked this pull request as ready for review August 16, 2022 17:43
@@ -2256,6 +2253,9 @@ def can_compute(self, functions=None, log_levels=None, override=False):
False.

"""
# TODODASKAPI - this method is premature - needs thinking about as part
Copy link
Member Author

Choose a reason for hiding this comment

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

These methods marked TODODASKAPI are API-related and therefore can probably be bumped to be considered instead as part of work to do after the Dask migration release, i.e. tranform to plain TODOs that we can keep track of via a dedicated Issue. I'll note that on the main Issue #433.

@sadielbartholomew sadielbartholomew merged commit 96c6a76 into NCAS-CMS:lama-to-dask Aug 16, 2022
@sadielbartholomew sadielbartholomew deleted the lama-to-dask-even-more-tidying branch August 16, 2022 17:48
@davidhassell davidhassell added this to the 3.14.0 milestone Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code tidy dask Relating to the use of Dask
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants