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

Add read/write support for wave reference waves in JSON wave notes #1989

Conversation

t-b
Copy link
Collaborator

@t-b t-b commented Jan 23, 2024

No description provided.

@t-b
Copy link
Collaborator Author

t-b commented Jan 25, 2024

Results from review from @MichaelHuth:

  • Add two variants for reading one for contained numeric and one for contained text
  • Extend tests for text waves and read/write of empty/null waves
  • assert on invalid waves types and contained null waves

@t-b t-b assigned MichaelHuth and unassigned t-b Jan 29, 2024
@t-b t-b force-pushed the feature/1989-add-support-for-reading-writing-wave-reference-waves-for-JWN branch from 7ea6447 to f3646d7 Compare January 29, 2024 14:58
We would fail later on due to the IsNumericWave/IsTextWave checks anyway
but the error message is better.
@t-b t-b force-pushed the feature/1989-add-support-for-reading-writing-wave-reference-waves-for-JWN branch 2 times, most recently from 92ebc5a to e28ad6a Compare January 29, 2024 19:08
@t-b t-b assigned t-b and MichaelHuth and unassigned MichaelHuth and t-b Jan 29, 2024
@t-b
Copy link
Collaborator Author

t-b commented Jan 29, 2024

Ready for review.

@MichaelHuth
Copy link
Collaborator

Looks good to me.

The extra Make to do the null wave check seems to be not very efficient, though there doesn't seem to be an obvious better method. I thought of FindValue but that does not accept WREF waves.

@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Jan 31, 2024
MichaelHuth
MichaelHuth previously approved these changes Jan 31, 2024
@t-b
Copy link
Collaborator Author

t-b commented Jan 31, 2024

Looks good to me.

The extra Make to do the null wave check seems to be not very efficient, though there doesn't seem to be an obvious better method. I thought of FindValue but that does not accept WREF waves.

I've removed the make invocation and used our GetRowIndex wrapper.

$ git range-diff @{u}...HEAD
1:  e28ad6af0 ! 1:  695662449 MIES_JSONWaveNotes.ipf: Add read/write support for wave reference waves
    @@ Packages/MIES/MIES_JSONWaveNotes.ipf: threadsafe Function/WAVE JWN_GetTextWaveFr
     +
     +   JSON_Release(jsonID)
     +
    -+   Make/FREE/N=(numRows) junk = WaveExists(container[p])
    -+
    -+   ASSERT_TS(Sum(junk) == numRows, "Encountered invalid waves in the container")
    ++   ASSERT_TS(IsNaN(GetRowIndex(container, refWave = $"")), "Encountered invalid waves in the container")
     +
     +   return container
     +End

@t-b t-b force-pushed the feature/1989-add-support-for-reading-writing-wave-reference-waves-for-JWN branch from e28ad6a to 6956624 Compare January 31, 2024 14:05
@t-b t-b merged commit 24aa612 into main Feb 1, 2024
19 checks passed
@t-b t-b deleted the feature/1989-add-support-for-reading-writing-wave-reference-waves-for-JWN branch February 1, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants