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

update 3d viewer, and json-dataset #138

Merged
merged 21 commits into from
Jul 12, 2023
Merged

Conversation

toloudis
Copy link
Contributor

@toloudis toloudis commented Jun 28, 2023

Problem

Internal scientists are again requesting to use onprem datasets. So we need to make sure the json-dataset code is working. Also discovered that the current 3d viewer was out of sync and was failing to load volume data.

Solution

Update the onprem datastore.
"http://dev-aics-dtp-001.corp.alleninstitute.org/cfedata/cell-feature-data/datasets.json" ( true location is //allen/aics/animated-cell/Allen-Cell-Explorer/cell-feature-data/datasets.json ) now must hold a list of subdirectories which have datasets in the format prescribed in allen-cell-animated/cell-feature-data repo.

Currently CFE is updated to only support the "single dataset" version and megasets are not yet being handled. Most of the interesting changes are in json-dataset/index.ts.
Also issued new 3d viewer releases and updated 3d viewer version to confirm that data still loads.

This is phase 1. Phase 2 would be to add support for megasets, but it is likely that for internal use the presentation of megasets is not yet important.

Steps to Verify:

http://dev-aics-dtp-001.corp.alleninstitute.org/cell-feature-explorer/dist/index.html
now points to this branch, which loads raw datasets from the above mentioned onprem directory (the 3d image data is still in cloud)

@toloudis toloudis requested a review from meganrm as a code owner June 28, 2023 16:30
megasetInfo.datasets = {
[id]: {
...topLevelJson,
image: `${datasetdirpath}/${topLevelJson.image}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

built-in assumption that the image file is local to the current dataset.json file

Comment on lines 98 to 107
public getAvailableDatasets = () => {
return axios
.get(`${this.listOfDatasetsDoc}`)
.then((metadata: AxiosResponse) => metadata.data);
//const megasets: Megaset[] = [];
return axios.get(`${this.listOfDatasetsDoc}`).then((metadata: AxiosResponse) => {
const datadir = this.listOfDatasetsDoc.substring(
0,
this.listOfDatasetsDoc.lastIndexOf("/")
);

return Promise.all(
(metadata.data as string[]).map((datasetdir) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the top-level function and the argument to map could become async. By no means necessary, but would save a couple indents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, I went ahead and fixed that

Comment on lines 118 to 125
} else {
const topLevelJson = { ...megaset } as unknown as DatasetMetaData;
const megasetInfo = { ...megaset };
megasetInfo.title = topLevelJson.title;
megasetInfo.publications = megaset.publications || [];
megasetInfo.extra = megaset.extra || "";
const id = `${topLevelJson.name}_v${topLevelJson.version}`;
megasetInfo.name = id;
Copy link
Contributor

@frasercl frasercl Jun 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block starts with three copies of the same object (megaset, megasetInfo, topLevelJson) and copies properties between them.

  1. I see megasetInfo is eventually returned from here, and it looks like topLevelJson is there to keep an unmodified copy around to copy into megasetInfo.datasets below. Could megaset be copied in directly?
  2. Looks like the title assignment on 121 may be redundant.
  3. Suggestion: some of the remaining property assignments which just apply defaults could happen at assignment of megasetInfo, e.g.
const megasetInfo = {
    publications: megaset.publications || [],
    extra: megaset.extra || "",
    // ...
    ...megaset
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is now addressed. It is much simpler now.

@@ -90,7 +90,7 @@ class FirebaseRequest implements ImageDataset {
});
};

public setCollectionRef = (id: string) => {
private setCollectionRef = (id: string) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only substantive change in this file is to change this from public to private. The rest of the file just got auto-formatted.

Copy link
Contributor

@ShrimpCryptid ShrimpCryptid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your code comments!

@toloudis toloudis merged commit 09b426d into main Jul 12, 2023
1 check passed
@toloudis toloudis deleted the feature/support-cell-feature-data branch July 12, 2023 15:52
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.

None yet

3 participants