Interface for data operations - #101
Conversation
prasad-sawantdesai
left a comment
prasad-sawantdesai
left a comment
There was a problem hiding this comment.
Are we also suppose to support operations on signals (like comparing two signals)
Yes, this is the binary operations mentioned by @mfiaudrin-epsyl and depending on #90 being merged |
|
Should we guard sigma input (not less than or equal to "0")? 
INFO: 127.0.0.1:45640 - "GET /data/plot_data?uri=imas%3Ahdf5%3Fpath%3D%2Fwork%2Fimas%2Fshared%2Fimasdb%2FITER%2F3%2F100120%2F1%23summary%3A0%2Fglobal_quantities%2Fip%2Fvalue&interpolation_method=exact_value&smoothing_method=gaussian_filter&gaussian_smoothing_sigma=0 HTTP/1.1" 500 Internal Server Error
ERROR: Exception in ASGI application
Traceback (most recent call last):
File "/home/ITER/sawantp1/.local/lib/python3.11/site-packages/uvicorn/protocols/http/httptools_impl.py", line 421, in run_asgi
.
^^^^^^^^^^^^^^^^^^^^^
File "/home/ITER/sawantp1/github/IBEX/backend/ibex/endpoints/data.py", line 120, in plot_data
return CustomORJSONResponse(ibex_service.get_plot_data(plot_data_query))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ITER/sawantp1/github/IBEX/backend/ibex/core/ibex_service.py", line 121, in get_plot_data
return data_source.get_plot_data(plot_data_query)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ITER/sawantp1/github/IBEX/backend/ibex/data_source/imas_python_source.py", line 1103, in get_plot_data
data_to_be_returned = apply_gaussian_filter(
^^^^^^^^^^^^^^^^^^^^^^
File "/home/ITER/sawantp1/github/IBEX/backend/ibex/data_source/imas_python_source_utils.py", line 178, in apply_gaussian_filter
return gaussian_filter1d(data, sigma=sigma)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ITER/sawantp1/.local/lib/python3.11/site-packages/scipy/ndimage/_filters.py", line 752, in gaussian_filter1d
weights = _gaussian_kernel1d(sigma, order, lw)[::-1]
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ITER/sawantp1/.local/lib/python3.11/site-packages/scipy/ndimage/_filters.py", line 665, in _gaussian_kernel1d
phi_x = np.exp(-0.5 / sigma2 * x ** 2)
~~~~~^~~~~~~~
ZeroDivisionError: float division by zero |
There was a problem hiding this comment.
Looks like changing data smoothing of one signal applies to both. Is it correct behavior?
Original
Applied downsampling and data smoothing
There was a problem hiding this comment.
Hello @prasad-sawantdesai,
The behavior you mention is unexpected. In the last version of my branch feature/data_operations, the data smoothing is only applied to the selected plot.
Can you please confirm if your behavior is occuring in the updated feature/data_operations branch ? If it is, I will need your configuration to reproduce it.
I can prevent from having 0 in sigma field because it's a quick improvement. I will it do it when I will have missing endpoints from the BE. |
No need for the time being, as mentioned in #101 (comment) |
|
Hello @olivhoenen , Following the toolchain version update, I need PR #116 to be approved and merged into the “develop” branch. That way, I can run the application on that branch to implement the binary operations. |
Ready now for you |
Thank you I am on it |
|
@olivhoenen, I just implement the operations on signals. The data operation has been finalized. Now we can choose between unary or binary operations. The PR is ready for review. |
|
@prasad-sawantdesai , @olivhoenen, |
|
Thanks @mfiaudrin-epsyl, concentrating here on data operations as the other data manipulations were tested before and looked fine to me. There are a few issues/questions on the data operations implementation here, tested from this config
|
olivhoenen
left a comment
olivhoenen
left a comment
There was a problem hiding this comment.
I see a regression in this PR, inside the metadata menu with the config test-signal-proc2IbexState(1).json:
when I click on tabs for URI-1 or URI-2, it returns automatically to URI-0
This is not happening with the same config on develop.
Thank you for your feedbacks. About the regression you have seen, It's a bug that we didn't reproduce before because it's also in develop. It's occuring when we have a graph in edition mode. In this case, we have the unexpected behavior. After some tests, I confirm that the bug is occuring only in metadata component. I will let you know when it'll be fixed. |
Thanks for the detailed review. Answers point by point:
This isn't a UI choice, it's a backend constraint. The two features (unary and binary operations) travel through two distinct query parameters ( So the request order can't be honoured whatever the frontend sends. Lifting this would require a backend change: a single ordered parameter where each entry is either I don't know if it it's doable in this scope or if it's a feature. I'd suggest tracking that as a separate backend issue rather than folding it into this PR. In the meantime the UI only shows that warning when both kinds are actually mixed.
You're right, and it's on the frontend side. The backend is fine, it does return the updated unit in the response. The frontend stores it on the plot, but the unit displayed in the axis title and in the metadata panel is read from the axis ( However, a new unit coming out of an operation causes a problem, because we currently have a hard limit of 2 Y axes, hence 2 different units per grid. To display the result of an operation, that constraint has to go. This is doable: plotly.js supports more than 2 Y axes, and the version we bundle handles the positioning of the extra axes automatically. The trade-off is readability: a grid with many Y axes becomes harder to read, and each extra axis eats into the plot area. Here is what it would look like: As a side benefit, this also lifts the limitation when adding signals from the tree: users would no longer be capped at 2 different units. One thing to note: saved configurations currently store the two axes explicitly, so I'd update the configuration structure to allow to have N y axes. @olivhoenen please confirm you'd like me to implement this.
After reviewing the issue, it appears that this will require significant changes to both the back end and the front end. The application is currently built on the assumption that every displayed signal comes from a data source: a node selected in the tree, with its own URI, name (by default), unit and metadata. That link to a source is what drives the plot's name, what the metadata panel displays, what gets persisted in a configuration and what is reloaded when the configuration is opened again. A signal produced by an operation has no such source. It exists only as the result of a computation. Supporting a signal that stands on its own means adapting across the whole chain: naming, tabs, metadata, customization and persistence. This is a real design change rather than an addition, and it isn't work we had anticipated. There may be a backend aspect as well, and I'd rather not state anything definitive here. From what I could see, the operands of a signal operation are re-fetched from their URI, which suggests that a signal that is itself the result of an operation may not be usable as an operand. Chaining from the original signal seems fine — A * B / D can be expressed as an ordered list of operations applied to A. But referring to the result itself, for instance C / D, or combining two computed signals C1 / C2, might require an API evolution. @jwasikpsnc could you confirm whether that's the case? Given all this, I'd rather keep A op B = A in this PR and open a dedicated issue for A op B = C, so we could scope the frontend and backend work properly and implement it at a later time. Would that work for you?
Agreed, I'll apply both suggestions: rename the field description to "Select the operand", and add a short explanation of what A and B are for signal operations. About selecting A through the tabs: that logic is a direct consequence of A op B = A, so it would naturally disappear when point 3 will be implemented: unary and binary operations will no longer update signals they used as operands. So, A and B would both be picked explicitly when creating C. The solution you propose to improve the UX is a temporary solution and will be replaced when point 3 will be done. @imbeauf FYI |
Fine by me, @jwasikpsnc please confirm so I can make an issue for reference |
Before going to a much more general solution (which would be a new feature request/enhancement), we should still be able right now to have the new field/unit at the second y-axis (assuming a single unit for all curves initially on the plot), and error otherwise. |
I see the current limitation of the interactions with the backend, and will make a separate issue for ref. Nevertheless, having the capability at the frontend side to change the label of each curve/signal (was discussed in the past, but so far only the change of title was implemented) shall be possible, hopefully not to hard to implement (without changing the backend) and have applications outside of this specific data transforms. I will make another issue specifically for one. |
3 participants
This PR implements the interface that allows you to perform two types of operations on data:
This PR also closes #104 by preserving transposition after a data manipulations (downsampling, interpolation, smoothing and unary operations)
You can test this feature in the data manipulation component. Simply open the corresponding accordion (data smoothing or unary operations) and give it a try:
Reminder
I need you to approve the PR #90 in order to implement a common interface that lets you choose between unary and binary operations.