josdejong / mathjs

An extensive math library for JavaScript and Node.js
https://mathjs.org
Apache License 2.0
14.41k stars 1.24k forks source link

Removing user-defined units #2081

Open bornova opened 3 years ago

bornova commented 3 years ago

Is there a way to remove user-defined units? I create units for currency conversion using:

math.createUnit('USD', {aliases: ['usd']})

var rates = currencyRates //Rates in json format
Object.keys(rates).map((currency) => {
    math.createUnit(rates[currency].code, {
        definition: math.unit(rates[currency].inverseRate + 'USD'),
        aliases: [rates[currency].code.toLowerCase()]
    }, {
        override: true
    })
})

But if I no longer want to use currencies, is there a way to remove them without creating a new mathjs instance (in my case without refreshing browser window)? If not, may I suggest introducing a math.removeUnit() method?

josdejong commented 3 years ago

A function like removeUnit is currently lacking, would be nice to add this 👍 .

@ericman314 do you have any thoughts about this? Any things you know that we should reckon with in this regard?

Anyone interested in picking this up?

Josef37 commented 3 years ago

There is a helper deleteUnit: https://github.com/josdejong/mathjs/blob/a3360d316ba6072661dd05678bfd26fdf9f674fe/src/type/unit/Unit.js#L3301-L3303

It gets used in createUnit when overriding: https://github.com/josdejong/mathjs/blob/a3360d316ba6072661dd05678bfd26fdf9f674fe/src/type/unit/Unit.js#L3081-L3093

We could extract this routine into it's own funciton. But I guess removing aliases automatically isn't possible right now, since they don't get recorded with the original unit.

josdejong commented 3 years ago

I think indeed we need to take care that some units are coupled to specific base units, so you can't just delete those base units without first cleaning up all depending units. So the exposed removeUnit function should check against that.

Josef37 commented 3 years ago

This could require some design decisions.

  1. Should we even allow deleting base units? If we do, we'd have to check all current units, if they depend on this base. And it could change indices for base dimensions, so I'd rather keep it around and only remove all derived units.
  2. I've looked at the unit design and all units get associated with a base unit and a value (like 1 meter = 1 LENGTH). So it would be no problem to remove units without checking (like the current deleteUnit).
  3. Should we provide an option to remove aliases?
    1. We could give that task to the user (since he created them).
    2. We could save each other unit alias in each unit alias and remove all of them.
    3. We could redesign aliases to not be its own unit in UNITS. Then we'd always call something like resolveUnitFromAlias before doing work with units. I'd prefer this option.
  4. Should we allow to only delete one alias? I'd leave this out.

What do you think, @josdejong?

josdejong commented 3 years ago

Thanks for thinking this through Josef!

  1. Yes I think it should be possible to delete base units. I think it should be possible to remove everything you've created via createUnit again via removeUnit. In case of deleting base units again, we should throw an error when there are still references. Probably we should silently remove any aliases if there are any (or do you think it would be better to throw an error and first force to clean up aliases?). You have a good point about the base dimensions being tight to a specific index. Maybe we can come up with a pragmatic solution there: only allow deleting the last one or so?
  2. 👍
  3. Yes I think so, see 1). Interesting idea to rethink how aliases are handled, but may be best to handle that as a separate topic to keep things focused
  4. I have the idea that in practice you simply want to delete a specific unit with all it's aliases, not remove a single alias or so. See also 1).
ericman314 commented 3 years ago

As a way to handle base units, a major refactoring could solve the issue of units depending on the indices of the base units in BASE_DIMENSIONS. Instead of a unit's dimensions being stored as an array of numbers, it could be stored as an object where each key is a base dimension. Removing a base dimension would become unnecessary, since there's no BASE_DIMENSIONS array to maintain. This has the added benefit of allowing you to perform operations on units that were created at different times, before or after you added new base dimensions. If a particular base unit property doesn't exist on a unit, it's assumed to be 0.

This is a change I could make immediately with https://github.com/ericman314/UnitMath, if you think we should instead focus efforts there. (Development on that has stalled for a couple reasons, 1) I hit a technical snag involving unit systems, 2) because of COVID all of my work is remote now. I'm doing 100% programming for my day job, and the last thing I want to do after a long day of programming is... more programming.)

josdejong commented 3 years ago

That sounds like a good plan @ericman314, thanks. I think we should not do such a big refactoring in Unit.js in mathjs, but to this in https://github.com/ericman314/UnitMath, since we want to integrate UnitMath into mathjs.

I guess we're all in the same corona boat, no worries 😄. Is there anyone who can help @ericman314 with this in UnitMath?

orelbn commented 1 week ago

@josdejong @ericman314

It's been a while since this issue was opened. Are there any updates on this?

Also, if we are waiting on UnitMath, can we have a simpler implementation (maybe two PRs, one for non-base units and a follow-up one for handling base units) for now?