Come to resolution for 0 charge on MCAS

[adam-a-a]

https://github.com/watertap-org/watertap/pull/1176/files#diff-4b5fe003ad0128ad3cc082b8d7c4d64296d8d634b04dcc00538592a59628866eR389-R391

Currently a warning is always logged if no charge data are provided, as a safeguard. However, if 0 charge is provided, an error is raised. A change should be made so that we (1) safeguard the novice user and also (2) don't annoy advanced users with warnings that are unnecessary for their purposes.

EDIT: deleting the logger warning is trivial. The true underlying issue at hand is that we need a way to tell the user that charge data need to be supplied for ions if they didn't provide the data. Seems obvious, but users acclimating to WaterTAP might not be aware of how to do so or that they have to do so. A user may also forget to do so.

[lbibl]

A reminder that not allowing zero charge assignment is bound to how the Set grouping is set up. I don't see the necessity of no-charge warning so recommend removing it.

[adam-a-a]

@lbibl what do you suggest for the case where the user specifies ions in the solute_list but fails to provide charge data? This is the underlying issue that we want to account for.

[adam-a-a]

My idea is to go back to allowing the user to specify 0 charge for neutral solutes without raising an exception (as we do now). Then, we can simply require the user to provide charge data for every component more easily by raising an exception when charge data are missing.

[adam-a-a]

@hunterbarber @kurbansitterley feel free to chime in

[lbibl]

I would not use “fail” for that case. Typically, those will be treated as neutral species and put in the neutral set. Errors will be triggered when they call the charge property for programmed neutral species. Avoiding using absolute zero is usually a good practice, especially , for instance, some charge may exist in denominators.

On Nov 16, 2023, at 3:43 PM, Adam Atia @.***> wrote:

@lbibl https://github.com/lbibl what do you suggest for the case where the user specifies ions in the solute_list but fails to provide charge data?

— Reply to this email directly, view it on GitHub https://github.com/watertap-org/watertap/issues/1200#issuecomment-1815504007, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXHWEICOBVUSUXU5K4UJKGDYE2QKHAVCNFSM6AAAAAA7LKBODGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJVGUYDIMBQG4. You are receiving this because you were mentioned.

[lbibl]

BTW, this decision was discussed and concluded by the electrochem team when Set grouping was set up.

[adam-a-a]

BTW, this decision was discussed and concluded by the electrochem team when Set grouping was set up.

Are you referring to a fairly recent meeting that I was not a part of, or the meeting that took place when you first proposed altering sets within MCAS (which I was in)? In either case, that decision is now up for debate again in this issue.

Can you point me to any expressions in the repo that divide by charge_comp and include neutral components in the index? We always want to avoid division by zero, but I would argue that we need not divide by charge_comp; either such expressions could be reformulated, or components within a neutral_setwould not be included in the index of that constraint.

Also, your response focused on the case of the neutral solute but did not answer my question. My question was "what do you suggest for the case where the user specifies ions in the solute_list but fails to provide charge data?" For example, if a user were to exclude charge data completely for a list of cations and anions, how would we catch that?

[andrewlee94]

@adam-a-a One word of caution on reformulating divisions by zero - converting a/b = c to a = b*c where c=0 converts a singularity into a potential degeneracy. You are far better excluding zeroes where possible.

[hunterbarber]

I would not use “fail” for that case. Typically, those will be treated as neutral species and put in the neutral set. Errors will be triggered when they call the charge property for programmed neutral species. Avoiding using absolute zero is usually a good practice, especially , for instance, some charge may exist in denominators.

The annoyance from a user perspective I was facing was that: I have a neutral solute that I am specifying in MCAS. If I do not specify charge, I receive a warning log that charge was not specified, and that species will be assigned to neutral_set. So warning implies maybe I wasn't specific enough. I go in to specify charge = {"neutral": 0}, but I get an error that I cannot specify charge for something contained in neutral_set. This may be something with the sequencing in the code.

So there is not a current way to specify a neutral species by charge without some terminal log (implying potential user error) in the current implementation.

[lbibl]

Discussion was conducted when Set grouping was set up in the weekly electrochem meeting. Don’t recall further detail.

Devision by charge is commonly encountered in electrochemical equations when charge equivalency is accounted for. I can’t point to expressions in the repo. Zeros should be avoided in other positions if possible too.

If that case causes a mistake, errors will be caught when their charge_comp is called in the model calculation.

On Nov 17, 2023, at 5:16 AM, Adam Atia @.***> wrote:

BTW, this decision was discussed and concluded by the electrochem team when Set grouping was set up.

Are you referring to a fairly recent meeting that I was not a part of, or the meeting that took place when you first proposed altering sets within MCAS (which I was in)? In either case, that decision is now up for debate again in this issue.

Can you point me to any expressions in the repo that divide by charge_comp and include neutral components in the index? We always want to avoid division by zero, but I would argue that we need not divide by charge_comp; either such expressions could be reformulated, or components within a neutral_set would not be included in the index of that constraint.

Also, your response focused on the case of the neutral solute but did not answer my question. My question was "what do you suggest for the case where the user specifies ions in the solute_list but fails to provide charge data?" For example, if a user were to exclude charge data completely for a list of cations and anions, how would we catch that?

— Reply to this email directly, view it on GitHub https://github.com/watertap-org/watertap/issues/1200#issuecomment-1816409524, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXHWEIGXS4TGZXJNYKEPXBLYE5PRJAVCNFSM6AAAAAA7LKBODGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJWGQYDSNJSGQ. You are receiving this because you were mentioned.

[adam-a-a]

@adam-a-a One word of caution on reformulating divisions by zero - converting a/b = c to a = b*c where c=0 converts a singularity into a potential degeneracy. You are far better excluding zeroes where possible.

@andrewlee94 OK, that's also a valid concern, but the truth is, with the way MCAS is configured, you could simply index your constraint by the ion_set, excluding the neutral_set (charge=0) components, thereby avoiding ever having to deal with 0s.

In fact, the ED model that @lbibl developed does just that anyway.

I propose that I make my suggested changes to MCAS, which are: 1) delete the raise ConfiguratioError when charge specified as 0 by the user and simply replace with an assignment to neutral_set. 2) delete the logger warning for when multiple solutes provided without charge data 3) add a raise ConfigurationError when charge data are not provided for each solute.

Following those changes, all tests would have to pass of course, and any issues that would arise with charge=0 would show up at that point.

Overall, I think it is better to safeguard the user against configuring the model incorrectly from the start rather than trying to anticipate operations with 0 values, which I would think is something that could be caught by usage of the DiagnosticToolbox, for example. As MCAS stands now, if I am a new user, I could skip providing charge data, and model properties could be solved, albeit to incorrect solutions when involving anything dependent on charge.

[lbibl]

There is a distinction between a neutral particle does not have a charge property vs. does charge = 0, and the former is more accurate. So I am against 3. and in favor of only 2.

On Nov 17, 2023, at 6:58 AM, Adam Atia @.***> wrote:

@adam-a-a https://github.com/adam-a-a One word of caution on reformulating divisions by zero - converting a/b = c to a = b*c where c=0 converts a singularity into a potential degeneracy. You are far better excluding zeroes where possible.

@andrewlee94 https://github.com/andrewlee94 OK, that's also a valid concern, but the truth is, with the way MCAS is configured, you could simply index your constraint by the ion_set, excluding the neutral_set (charge=0) components, thereby avoiding ever having to deal with 0s.

In fact, the ED model that @lbibl https://github.com/lbibl developed does just that anyway.

I propose that I make my suggested changes to MCAS, which are:

delete the raise ConfiguratioError when charge specified as 0 by the user and simply replace with an assignment to neutral_set. delete the logger warning for when multiple solutes provided without charge data add a raise ConfigurationError when charge data are not provided for each solute. Following those changes, all tests would have to pass of course, and any issues that would arise with charge=0 would show up at that point.

Overall, I think it is better to safeguard the user against configuring the model incorrectly from the start rather than trying to anticipate operations with 0 values, which I would think is something that could be caught by usage of the DiagnosticToolbox, for example. As MCAS stands now, if I am a new user, I could skip providing charge data, and model properties could be solved, albeit to incorrect solutions when involving anything dependent on charge.

— Reply to this email directly, view it on GitHub https://github.com/watertap-org/watertap/issues/1200#issuecomment-1816578324, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXHWEIFR6WOH5QNOWEWZTCLYE53SRAVCNFSM6AAAAAA7LKBODGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJWGU3TQMZSGQ. You are receiving this because you were mentioned.

[adam-a-a]

There is a distinction between a neutral particle does not have a charge property vs. does charge = 0, and the former is more accurate. So I am against 3. and in favor of only 2.

The underlying problem still remains then. We need a way to tell the user right away that they need to supply charge data for ions if they hadn't.

[lbibl]

ok. I can just vision your careful consideration for that case- the current setup will throw those into neutral set to exclude their chance to call a charge_comp, which is indexed by ion_set. There is always a chance of user negligence and, in this case, I would not compromise a very accurate implementation for this user negligence chance.

[lbibl]

Just back to how this is raised- in my mind, had one seen the warning as a user, if the species named in the warning is indeed neutral, one would be happy things are correct and wouldn’t act on assigning a charge 0; and only if one saw some intended ions are put in the neutral, one would double check charge inputs. I am neutral on having or not having this warning.

On Nov 17, 2023, at 5:33 AM, Hunter Barber @.***> wrote:

I would not use “fail” for that case. Typically, those will be treated as neutral species and put in the neutral set. Errors will be triggered when they call the charge property for programmed neutral species. Avoiding using absolute zero is usually a good practice, especially , for instance, some charge may exist in denominators.

The annoyance from a user perspective I was facing was that: I have a neutral solute that I am specifying in MCAS. If I do not specify charge, I receive a warning log that charge was not specified, and that species will be assigned to neutral_set. So warning implies maybe I wasn't specific enough. I go in to specify charge = {"neutral": 0} , but I get an error that I cannot specify charge for something contained in neutral_set. This may be something with the sequencing in the code.

So there is not a current way to specify a neutral species by charge without some terminal log (implying potential user error) in the current implementation.

— Reply to this email directly, view it on GitHub https://github.com/watertap-org/watertap/issues/1200#issuecomment-1816440780, or unsubscribe https://github.com/notifications/unsubscribe-auth/AXHWEIA42RNQ7R5OJYJDBG3YE5RTDAVCNFSM6AAAAAA7LKBODGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJWGQ2DANZYGA. You are receiving this because you were mentioned.