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

Results gets wrong label #1927

Open
grzanka opened this issue Jan 2, 2025 · 14 comments
Open

Results gets wrong label #1927

grzanka opened this issue Jan 2, 2025 · 14 comments

Comments

@grzanka
Copy link
Contributor

grzanka commented Jan 2, 2025

I've defined a simulation from scrach, loosely based on "Treatment plan example".
Then I've executed it on yaptide.c3.plgrid.pl (which runs now on master branches from ui and yaptide).

Results gets wrong labels. First plot should be titled dose, not fluence:
image

Grouping doesn't work:
image

Here is the output JSON, saved with the button in upper right corner:
image

I was able to load the results into https://yaptide.github.io/web_dev/ instance:
image

The results seemed to be loaded correctly, but the editor project state is missing:
image

@grzanka
Copy link
Contributor Author

grzanka commented Jan 2, 2025

Backend DB status:

ubuntu@yaptide:~$ docker exec -it yaptide_flask bash -c "flask --app yaptide.application db show"
Rev: 5003b9acb1f4 (head)
Parent: 41149fa64bd2
Path: /usr/local/app/migrations/versions/5003b9acb1f4_.py

    empty message
    
    Revision ID: 5003b9acb1f4
    Revises: 41149fa64bd2
    Create Date: 2024-12-10 22:51:14.262553

ubuntu@yaptide:~$ docker exec -it yaptide_flask bash -c "flask --app yaptide.application db check"
INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.ddl.postgresql] Detected sequence named 'Estimator_id_seq' as owned by integer column 'Estimator(id)', assuming SERIAL and omitting
INFO  [alembic.ddl.postgresql] Detected sequence named 'Cluster_id_seq' as owned by integer column 'Cluster(id)', assuming SERIAL and omitting
INFO  [alembic.ddl.postgresql] Detected sequence named 'Logfiles_id_seq' as owned by integer column 'Logfiles(id)', assuming SERIAL and omitting
INFO  [alembic.ddl.postgresql] Detected sequence named 'Page_id_seq' as owned by integer column 'Page(id)', assuming SERIAL and omitting
INFO  [alembic.ddl.postgresql] Detected sequence named 'Input_id_seq' as owned by integer column 'Input(id)', assuming SERIAL and omitting
No new upgrade operations detected.
ubuntu@yaptide:~$ date
Thu Jan  2 10:44:59 UTC 2025

@jagodek jagodek self-assigned this Jan 2, 2025
@jagodek
Copy link
Contributor

jagodek commented Jan 2, 2025

@grzanka. Json with project would be helpful to recreate issue.

@grzanka
Copy link
Contributor Author

grzanka commented Jan 2, 2025

Here is the results JSON - is it enough ?
treatment-plan-160-mev-mono-result.json.json

@jagodek
Copy link
Contributor

jagodek commented Jan 2, 2025

I meant json saved from 3d editor

@grzanka
Copy link
Contributor Author

grzanka commented Jan 2, 2025

Something is weird, once I click "Load to Editor" I get nothing in the editor pane. Then if I save this to a hard drive I get following, almost empty file:
plan160mev.json

You can find all necessary data (bodies, zones, filters, scorers) in the JSON from this comment:
#1927 (comment)
Unfortunately for some reason it doesn't load into editor :(

I will try to recreate from scratch a smaller example to reproduce this issue.

@grzanka
Copy link
Contributor Author

grzanka commented Jan 2, 2025

Here is a new one. Check results for "ZPROFILE"
wrong.json

Results.-.Google.Chrome.2025-01-02.16-51-01.mp4

@jagodek
Copy link
Contributor

jagodek commented Jan 2, 2025

So far i noticed that something wrong is with customMaterial when loading project from results
YaptideEditor.js:711 Error: SimulationMaterial not found
at rZ.reconstructMaterialFromJSON (SimulationZone.ts:122:47)
at rZ.fromJSON (SimulationZone.ts:204:8)
at rZ.fromJSON (BooleanZone.ts:203:9)
at rZ.fromJSON (BooleanZone.ts:216:15)
at pZ._loader (ZoneManager.ts:25:54)
at SimulationContainer.ts:87:23
at Array.forEach ()
at pZ.fromJSON (SimulationContainer.ts:86:8)
at fZ.fromJSON (ZoneManager.ts:220:22)
at _Q.fromJSON (YaptideEditor.js:677:38)
and it's from this material:
"customMaterial": {
"color": 9744820,
"density": 0.00120479,
"icru": 104,
"name": "AIR, DRY (NEAR SEA LEVEL)",
"opacity": 0,
"originalMaterialUuid": "be881070-c34e-4a41-83ca-2110cb0395b7",
"sanitizedName": "air_dry_near_sea_level",
"transparent": true,
"uuid": "8753929f-be20-4958-8edc-bba0b9cc3ba4"
},

@jagodek
Copy link
Contributor

jagodek commented Jan 2, 2025

Yup. Seems like AIR, DRY (NEAR SEA LEVEL) has no entry in materials unlike WATER, LIQUID does.

@grzanka
Copy link
Contributor Author

grzanka commented Jan 2, 2025

Yup. Seems like AIR, DRY (NEAR SEA LEVEL) has no entry in materials unlike WATER, LIQUID does.

Why not ? Air is here:

{
icru: 104,
name: 'AIR, DRY (NEAR SEA LEVEL)',
sanitized_name: 'air_dry_near_sea_level',
density: 0.00120479
},

@jagodek
Copy link
Contributor

jagodek commented Jan 2, 2025

Here is the results JSON - is it enough ? treatment-plan-160-mev-mono-result.json.json

I'm talking about this file, the customMaterial has originalMaterialUuid field which refers to material in materialManager>materials which is not there.

@grzanka
Copy link
Contributor Author

grzanka commented Jan 2, 2025

Here is the results JSON - is it enough ? treatment-plan-160-mev-mono-result.json.json

I'm talking about this file, the customMaterial has originalMaterialUuid field which refers to material in materialManager>materials which is not there.

Can you check as well this one ?
wrong-result.json.json

These results correspond to #1927 (comment)

When I open this JSON file in https://yaptide.github.io/web_dev/ then the results are loaded correctly:
image

The same simulation card on which I clicked "Save to file" on https://yaptide.c3.plgrid.pl/ loads them incorrectly.
image

Main difference between https://yaptide.github.io/web_dev/ and https://yaptide.c3.plgrid.pl/ is that the latter one doesn't have access to the backend.

@grzanka
Copy link
Contributor Author

grzanka commented Jan 2, 2025

This is how yaptide.c3.plgrid.pl loads results, when I click on ZPROFILE it makes a call to backend:

image

@grzanka
Copy link
Contributor Author

grzanka commented Jan 2, 2025

What I suspect is that there was a nice fix by @SzymanskiBartlomiej in #1898 , merged on 12.12

Then same day, but later, then came a feature by @matuszsmig in #1884, merged same day, but after the "filter-fix".

Both PRs modified src/services/ShSimulatorService.tsx which contains crucial logic.

Can you check if after changes by @matuszsmig recreateRefToScoringManagerOutputs and recreateRefToFilters is still in place and works ?

image

@grzanka
Copy link
Contributor Author

grzanka commented Jan 3, 2025

After digging a bit I see a difference: getEstimatorsPages was introduced in #1884
It calls recreateRefsInResults on a single element list containing one estimator

	const getEstimatorsPages = useCallback(
		async (...[info, signal, cache = true, beforeCacheWrite]: RequestGetJobResult) => {
                        (....)
			const cachePromise = resultsCache.createPromise(
                        (....)
					const estimator: Estimator[] = [
						{
							name: estimatorName,
							pages: response.pages
						}
					];

					updateEstimators(estimator);

					const jobInputs = await getJobInputs(info, signal, cache);

					const refsInResults =
						jobInputs?.input.inputJson &&
						recreateRefsInResults(jobInputs.input.inputJson, estimator);

					const data: SpecificEstimator = {
						jobId,
						estimators: refsInResults ?? estimator,
						message: response.message
					};

Previously used method to fetch the result (prior to #1884), used recreateRefsInResults with list of all estimators.

From quick look on recreateRefsInResults and how it calls recreateRefToScoringManagerOutputs I guess that it needs to get JSON with same arrays of same length containing estimators. There is a lot of index-based logic which gets wrong in some cases if we pass different number of estimators.

See recreateRefsInResults:

export const recreateRefsInResults = (inputJson: EditorJson, estimators: Estimator[]) => {
if (!inputJson) throw new Error('No editor data');
if (!estimators) throw new Error('No esitamtors data');
const { scoringManager }: EditorJson = inputJson;
const estimatorsWithScoringManagerOutputs = recreateRefToScoringManagerOutputs(
estimators,
scoringManager
);
const estimatorsWithFixedFilters = recreateRefToFilters(
estimatorsWithScoringManagerOutputs,
scoringManager.filters
);
return estimatorsWithFixedFilters;
};

And recreateRefToScoringManagerOutputs

const recreateRefToScoringManagerOutputs = (
estimators: Estimator[],
scoringManagerJSON: ScoringManagerJSON
): Estimator[] => {
estimators.forEach((estimator, index) => {
estimator.scoringOutputJsonRef = scoringManagerJSON.outputs[index];
});
return estimators;
};

image

grzanka added a commit that referenced this issue Jan 3, 2025
@jagodek jagodek removed their assignment Jan 9, 2025
grzanka added a commit that referenced this issue Jan 11, 2025
* Fix results label in simulation

Related to #1927

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/yaptide/ui/issues/1927?shareId=XXXX-XXXX-XXXX-XXXX).

* Modify `getEstimatorsPages` callback to log `inputJsonForThisEstimator` and its `outputs` field

* Add console logs for `inputJsonForThisEstimator` and its `outputs` field

* Modify `getEstimatorsPages` callback to handle `inputJson` and `outputs` field

* **Recreate references functions**
  - Document `recreateRefsInResults`, `recreateRefToFilters`, and `recreateRefToScoringManagerOutputs` functions

* **Error message**
  - Fix typo in error message from "esitamtors" to "estimators"

* **Input JSON handling**
  - Make a copy of `jobInputs.input.inputJson` and assign it to `inputJsonForThisEstimator`
  - Ensure `inputJsonForThisEstimator` contains a field called `outputs`
  - Pass a single-element list as `outputs` to `recreateRefsInResults`
  - Dump `inputJsonForThisEstimator` to the console, particularly its `outputs` field

* Modify `getEstimatorsPages` callback to handle `inputJson` copy and pass filtered outputs

* Make a copy of `jobInputs.input.inputJson` and assign it to `inputJsonForThisEstimator`
* Ensure `inputJsonForThisEstimator` contains a field called `outputs`
* Pass a single-element list as `outputs` to `recreateRefsInResults`
* Update `data` object and `resolve` logic to handle the new `inputJsonForThisEstimator`

* Modify `getEstimatorsPages` callback to handle `inputJson` and add logging

* Make a copy of `jobInputs.input.inputJson` and assign it to `inputJsonForThisEstimator`
* Ensure `inputJsonForThisEstimator` contains a field called `outputs`
* Pass a single-element list as `outputs` to `recreateRefsInResults`
* Add logging for `inputJsonForThisEstimator` in the console
* Add debugging logging to console in `recreateRefToScoringManagerOutputs`, `recreateRefToFilters`, and `recreateRefsInResults`
* Add comments explaining code logic in `recreateRefToScoringManagerOutputs`, `recreateRefToFilters`, and `recreateRefsInResults`

* fixes

* cleaning

* more comments

* Update src/services/ShSimulatorService.tsx
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

No branches or pull requests

2 participants