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

Constructing spatial neighborhood with custom parameters #126

Merged
merged 21 commits into from
Dec 15, 2023

Conversation

Qirongmao97
Copy link
Contributor

No description provided.

@niklasmueboe
Copy link
Collaborator

I think this should rather be multiple scripts, one per methodology; radius, delaunay, ... rather than having all as one script just because squidpy decided to have them represented by a single function. This would make tracking differently generated neighborhoods easier.

Copy link
Collaborator

@niklasmueboe niklasmueboe left a comment

Choose a reason for hiding this comment

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

General comment for all cases;

I think arguments such as radius, n_neighbors should be moved to the config rather than in the CLI

preprocessing/neighbors/coord_type.py Outdated Show resolved Hide resolved
preprocessing/neighbors/delaunay_triangulation.py Outdated Show resolved Hide resolved
preprocessing/neighbors/n_neighs.py Outdated Show resolved Hide resolved
preprocessing/neighbors/radius.py Outdated Show resolved Hide resolved
preprocessing/neighbors/n_rings.py Outdated Show resolved Hide resolved
preprocessing/neighbors/n_neighs.py Outdated Show resolved Hide resolved
@niklasmueboe
Copy link
Collaborator

Are the spatial_distances matrices actually needed at all?

@Qirongmao97
Copy link
Contributor Author

General comment for all cases;

I think arguments such as radius, n_neighbors should be moved to the config rather than in the CLI

Just wondering if it is still necessary to separate them into different files based on different functions since the parameters could be set up all at once in the config file now.

@Qirongmao97
Copy link
Contributor Author

Are the spatial_distances matrices actually needed at all?

I will then remove them from the script.

@niklasmueboe
Copy link
Collaborator

General comment for all cases;
I think arguments such as radius, n_neighbors should be moved to the config rather than in the CLI

Just wondering if it is still necessary to separate them into different files based on different functions since the parameters could be set up all at once in the config file now.

I would still keep it separate because it is easier to track that you have e.g. neighbors defined using a radius if there is a specific script for that rather than having to parse the json file and then looking into the logic that is actually implemented when using these parameters

parameters = json.load(f)

n_neighs = data["n_neighs"]
f.close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

ContextManager will automatically close the file. This line is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in all scripts

@niklasmueboe
Copy link
Collaborator

You should also use a config file for the other scripts to pass radius etc. Make sure that the scripts and the yaml files have the same name so it is clear which ones belong together

@Qirongmao97
Copy link
Contributor Author

You should also use a config file for the other scripts to pass radius etc. Make sure that the scripts and the yaml files have the same name so it is clear which ones belong together

Config file added

@Qirongmao97
Copy link
Contributor Author

I noticed that in #154 spatial distance metric is needed as input.

Copy link
Collaborator

@niklasmueboe niklasmueboe left a comment

Choose a reason for hiding this comment

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

Configs should be per script not for all at the same time. Laso I think now that we expanded this we should move each script with its yaml and config into a subdirectory. Should be an easy change. Maake sure to put each config also into a subdirectory called config.

Copy link
Collaborator

@niklasmueboe niklasmueboe Dec 15, 2023

Choose a reason for hiding this comment

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

Why delaunay: false. I think for this one we won't need a config at all. And delaunay should always be true

@niklasmueboe niklasmueboe merged commit 8edfdd3 into main Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constructing spatial neighborhood with custom parameters
2 participants