Refactor nxsunit #13
Merged
Refactor nxsunit #13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…pdate unit tests to match
… units into a list
…t tests to match functionality
…ct calculation issues found during testing.
|
Still waiting for review by @pkienzle and @lucas-wilkins (though the latter will be after Jan 1 I guess?). @wpotrzebowski may also take a look. |
|
|
||
| # Find the scale for the given units - default to dimensionless | ||
| self.scalemap = [DIMENSIONS.get(dimension, DIMENSIONS['dimensionless']) for dimension in self.dimension] | ||
| base = self._get_scale_for_units(self._units) |
There was a problem hiding this comment.
Maybe better to return scalebase and scaleoffset as two separate variables?
lucas-wilkins
approved these changes
Jan 4, 2023
| return map | ||
| units = {} | ||
| units.update([(name, scale) for name, scale in kw.items()]) | ||
| units.update([(name + 's', scale) for name, scale in kw.items()]) |
There was a problem hiding this comment.
I know you've not really changed anything here, but plurals are not always made by adding an 's'. e.g. inches.
There was a problem hiding this comment.
I agree, but this class works mostly in SI units. Here is a list of edge cases we might come up against where this fails, based on my understanding of the NIST guide to SI:
- Anything Imperial that isn't a temperature
- Potential solution: add imperial lengths and weights
- What I am willing to do for this PR: I will continue to omit this. The scattering community doesn't work in inch(es)/feet/pounds/etc.
microohmandkiloohmwill be generated by the class, butmicrohmandkilohmare the correct forms.- Potential solution: explicitly add these as resistance units
- What I am willing to do for this PR: I could implement this pretty quickly if there is a call for it.
- units in long-form, e.g.
meters per second squared-or-meters per square second- Potential solution 1: convert
<square|cubic> <unit>or<unit> <squared|cubed>tounit^<2|3> - Potential solution 2: convert
perto/ - What I am willing to do for this PR: I ignored this because long-form is typically reserved for textual content, not data units, but implementation would be fairly quick. I'd also need to add some reasonable unit tests.
- Potential solution 1: convert
|
is this ready to merge? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This overhauls the nxsunit module to allow scaling with offsets, and scaling of multi-dimensional and complex units, e.g. SESANS A^-2*cm^-1 will treat the A^-2 and cm^-1 as separate components, each independently scalable. Unit tests were moved from within the module to the global unit test suite.
Fixes #5
Fixes(?) SasView/sasview#2035 - @pkienzle what do you think?