Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DecayModel.halfLifeNumberProperty doesn't support "unknown" half lives #111

Closed
zepumph opened this issue Aug 1, 2023 · 5 comments
Closed
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Aug 1, 2023

Found while handling a TODO in #93. If unknown, then that Property should be null (from AtomIdentifier), but that isn't supported or expected from its TypeScript implementation.

@zepumph
Copy link
Member Author

zepumph commented Aug 2, 2023

It isn't clear what the desired behavior is for the halfLifeNumberProperty when it reaches unknown. @Luisav1 let me know if you want assistance on this.

@zepumph zepumph removed their assignment Aug 2, 2023
@Luisav1
Copy link
Contributor

Luisav1 commented Aug 9, 2023

If the half-life is null (aka "unknown") then the half-life is set to -1 when the halfLifeNumberProperty is set.

// the nuclide is unstable and its half-life data is missing, set -1 as the half-life as a placeholder
else if ( AtomIdentifier.getNuclideHalfLife( protonNumber, neutronNumber ) === null ) {
halfLife = -1;
}

Then this is dealt with appropiatly in the listener:

// the nuclide is unstable but the half-life data is unknown
else if ( halfLifeNumber === -1 ) {
showHalfLifeArrow( false );
halfLifeUnknownText.visible = true;
halfLifeNumberText.visible = false;
this.moveHalfLifePointerSet( 0, options.isHalfLifeLabelFixed );
}

The use of -1 as a placeholder can be done in AtomIdentifier instead though like below, is this something we want to do?

  // Get the half-life of a nuclide with the specified number of protons and neutrons.
  getNuclideHalfLife: function( numProtons: number, numNeutrons: number ): number | null | undefined {
    if ( !HalfLifeConstants[ numProtons ] ) {
      return undefined;
    }
    else if ( HalfLifeConstants[ numProtons ][ numNeutrons ] === null ) {
      return -1;
    }
    return HalfLifeConstants[ numProtons ][ numNeutrons ];
  },

@zepumph
Copy link
Member Author

zepumph commented Aug 15, 2023

I think the issue to solve here is that the terms undefined and null are codified values that mean something, but are not documented. I believe that the work to be done is to document the possible results returned by the function (a half life number, no half life, or an unknown half life), and then the corresponding code values of each. I think of it as a bug that undefined and null mean two different things, and I think that -1 would do a good job of representing an unknown half life.

Also if you would like my response or assistance on an issue please re-assign me so I don't lose it (which I am very good at doing).

Luisav1 added a commit that referenced this issue Aug 16, 2023
Luisav1 added a commit to phetsims/shred that referenced this issue Aug 16, 2023
@Luisav1 Luisav1 assigned zepumph and unassigned Luisav1 Aug 22, 2023
@zepumph
Copy link
Member Author

zepumph commented Aug 22, 2023

Discussed with @Luisav1, let's also change null -> -1 in HalfLifeConstants too.

  • Update doc
  • remove the mapping from null -> -1 in the getter function.

@zepumph
Copy link
Member Author

zepumph commented Aug 23, 2023

After finding this data null = unknown duplicated in the DECAYS_INFO_TABLE as well, I think it may just be easiest to keep things as is. Closing

@zepumph zepumph closed this as completed Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants