nufft dtype error about rm-tools HOT 6 CLOSED

Hey @ErikOsinga I see you've already committed a fix in c57c641.

It's probably better to open PRs for this type of change. Also please make sure you've got the dev tooling installed - especially the pre-commit checks.

If you're happy that the fix is working, would you mind adding a test to ensure we don't run into the same problem again?

from rm-tools.

Sure I can open PRs in the future, I was unsure whether it made sense for such small changes.

What's the 'dev tooling?' and how do I install that?

from rm-tools.

Additionally, sort of related to this dtype issue, for complex64 dtypes, the default threshold you put in of eps=1e-8 raises a Warning that it cannot be achieved and the tolerance is automatically increased to eps_mach=6e-08

I suggest raising the default to eps=1e-7 (or 1e-6 which is the finufft default), and we could make it a variable for input to do_rmsynth_planes() and get_rmsf_planes() as well. Thoughts?

from rm-tools.

Hey @ErikOsinga - no worries! Here's are relevant bits from the main README:

# After a local clone
cd RM-Tools
pip install ".[dev]" # Install dev dependencies
pre-commit install # Install pre-commit to apply rules

On the tolerances, that's a good catch. I agree with setting to the default, as well as opening up option as a parameter. I'm wondering whether we'd want to expose from the CLI. Perhaps for now we can juts add the parameter to the function and we can add a CLI arg down the line if we want. We'll just need to update the tests with the appropriate tolerance as well.

from rm-tools.

Does this eps parameter do anything interesting/important? Are there cases, aside from the different bit-depth, where a user would want to try different values?

Without having dug into it, my initial reaction is that if it isn't something that will matter to the majority of users then there's not really any value in making it a parameter, especially on the command line. I don't necessarily object to having it as a parameter for invoking the planes() functions (with a suitable default value), but I don't see the value in exposing it at the higher level functions -- advanced users who want to fuss with it can either write their own wrappers around the lower level functions or modify any hard-coded values inside a fork. I think the CLI is already busy enough to intimidate non-expert users, so I don't want to add any further complexity unless it's clearly well-motivated.

from rm-tools.

Just reopening this @Cameron-Van-Eck until we expose the param in the ..._planes functions and add some tests. Happy to take care of that

from rm-tools.