search

pik-piam / sodym

MIT License
0 stars 2 forks source link

split get_relative_mass_balance #22

Open JakobBD opened 1 month ago

JakobBD commented 1 month ago

The "total" shouldn't be in this function. Instead, there should be a function calling get_mass_balance and dividing it by a total.

JakobBD commented 1 month ago

Further:

SallyDa commented 1 week ago

Started working on this #43 but I have a couple of questions on the existing logic (see also comments in the pr). I guess dividing by the "total" mass balance is just to give us an idea of how important any imbalances are? It's not really clear to me why the total is defined the way it is.

JakobBD commented 1 week ago

Yes, the total is just meant to give a reference. Before, we had a hard-coded absolute tolerance. We meant to make this more universal by dividing by the total.
If check_mass_balance gets a tolerance as an argument, I'd also be okay with deleting get_mass_totals and defining that tolerance as an absolute one. But that would leave defining a meaningful tolerance to the user, which may be an obstacle.

Third option: as a reference, take max([np.max(np.absolute(f.values)) for f in self.flows.values()]) instead of get_mass_totals. That would be much shorter and probably just as good.