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

fix(raw): impossible to see matrix containing NaN values #1714

Merged

Conversation

MartinBelthle
Copy link
Contributor

@MartinBelthle MartinBelthle commented Aug 31, 2023

In the file raw_studies_blueprint.py, i "rollbacked" the behaviour as it was before this PR: #1577 to allow NaN values in matrices.

I added an integration test to ensure this worked.

I've also cleaned a bit the test_integration.py file:

  • I used the fixture admin_access_token when needed
  • I added a line admin_headers = {"Authorization": f"Bearer {admin_access_token}"} at the beginning of each test to use this value everywhere and by doing so, I lightened the code

@commit-lint
Copy link

commit-lint bot commented Aug 31, 2023

Bug Fixes

  • raw: allow NaN in matrices (0cad1a9)
  • api: allow NaN, +Infinity, and -Infinity values in JSON response (7394248)

Features

  • tests: add a test to ensure GET /raw endpoint reads NaN values (29b1f71)

Contributors

MartinBelthle, laurent-laporte-pro

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@MartinBelthle MartinBelthle linked an issue Aug 31, 2023 that may be closed by this pull request
@MartinBelthle MartinBelthle changed the title fix(raw): Impossible to see matrix containing NaN values fix(raw): impossible to see matrix containing NaN values Aug 31, 2023
@MartinBelthle
Copy link
Contributor Author

I tried to use ORJSONResponse but that converts NaN to None and we do not want that. Therefore, i keep the code as it is

@laurent-laporte-pro
Copy link
Contributor

laurent-laporte-pro commented Sep 5, 2023

I tried to use ORJSONResponse but that converts NaN to None and we do not want that. Therefore, i keep the code as it is

You are right: it is written in the documentation.
However, we could use orjson for its performance, but it doesn't work for those values 👎🏻

@laurent-laporte-pro laurent-laporte-pro force-pushed the fix/1659-impossible-to-see-some-matrix-in-output-files branch from 2e78b63 to b005e26 Compare September 5, 2023 16:42
@pull-request-size pull-request-size bot added size/L and removed size/XL labels Sep 5, 2023
@laurent-laporte-pro laurent-laporte-pro force-pushed the fix/1659-impossible-to-see-some-matrix-in-output-files branch from b005e26 to 79a141d Compare September 5, 2023 16:44
@laurent-laporte-pro laurent-laporte-pro self-assigned this Sep 21, 2023
@laurent-laporte-pro laurent-laporte-pro force-pushed the fix/1659-impossible-to-see-some-matrix-in-output-files branch from 79a141d to 7394248 Compare September 30, 2023 15:43
@pull-request-size pull-request-size bot added size/M and removed size/L labels Sep 30, 2023
@laurent-laporte-pro laurent-laporte-pro merged commit 1e7e1b1 into dev Sep 30, 2023
8 checks passed
@laurent-laporte-pro laurent-laporte-pro deleted the fix/1659-impossible-to-see-some-matrix-in-output-files branch September 30, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Impossible to see some matrix file in output
2 participants