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

Change main structure of the repo #8

Merged
merged 17 commits into from
Dec 19, 2022
Merged

Change main structure of the repo #8

merged 17 commits into from
Dec 19, 2022

Conversation

lauraporta
Copy link
Member

I want to simplify the project by having all the components in one repository.
Therefore I am starting to set the folder analysis/ which is going to contain the methods to be used to analyse the data.

I committed changes with a scaffold of the analysis methods (to be implemented) and a draft if the formatted_data class.

@lauraporta
Copy link
Member Author

Here on Canva I am making a sort of flow chart describing the desired package structure starting from the source code in MATLAB. The current version is attached as an image.

I would like first to set the overall design and then later fill the gaps (methods implementations).

I want to focus first on the pipeline regarding spatial and temporal frequency which is currently in use.

MAIN

@lauraporta
Copy link
Member Author

I completed the analysis of the matlab repo, here is a scheme which contains the important methods, classes, inputs and outputs:

MAIN

I divided the code in units which can be implemented and tested separately in python:
MAIN (1)

Now I can restructure the code following these schemas

@lauraporta
Copy link
Member Author

Related to the issue #11

@lauraporta lauraporta marked this pull request as ready for review December 12, 2022 13:31
@lauraporta
Copy link
Member Author

lauraporta commented Dec 12, 2022

Here a description of the overall new structure of the project. Many classes and methods are now just drafts and placeholders to be filled afterwards.

Main: the entry point

The entry point is main.py, which for now will work as a CLI and in the future could be wrapped in a TUI OR GUI.
These are the steps executed when calling main.py:

MAIN (1)

Modules and objects

The project is divided into folders which contain relevant modules.

  • Load: loads config, establishes paths to read on the server, loads the data
  • Objects: contains the definitions of
    • Config -> object holding location of files in the server, analysis options...
    • FolderNamingSpecs -> object dedicate to parse the input folder name
    • Options -> class replicating the options config file in the matlab project
    • PhotonData -> class containing the pre-processed data. When instantiated, it takes raw data as input and calls and preprocesses it.
  • Analysis: contains the main analysis classes and methods called for plotting
  • Plots: dedicated to plotting functions

Folder structure

.
├── __init__.py
├── analysis/
│   └── spatial_freq_temporal_freq.py
├── config/
│   └── config.yml
├── load/
│   ├── __init__.py
│   ├── load_data.py
│   └── read_config.py
├── main.py
├── objects/
│   ├── configurations.py
│   ├── folder_naming_specs.py
│   ├── options.py
│   ├── parsers/
│   │   ├── __init__.py
│   │   ├── parser.py
│   │   └── parser2pRSP.py
│   └── photon_data.py
├── plots/ 
│   └── plotter.py
└── utils.py

P.S. I am also addressing issue #9

@lauraporta lauraporta linked an issue Dec 12, 2022 that may be closed by this pull request
@lauraporta lauraporta changed the title Change main structure of the repo: add analysis folder Change main structure of the repo Dec 12, 2022
@niksirbi
Copy link
Member

Add types-PyYAML to dev dependencies in pyproject.yaml

Copy link
Member

@niksirbi niksirbi left a comment

Choose a reason for hiding this comment

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

Well done @lauraporta! I really like how you have structured the project, and I can definitely see adopting a similar structure for my projects too.
I have left some comments (most of them nitpicky, feel free to ignore).
On a higher level, it's a bit confusing to have config (as in config.yaml), Config (the class), and Options, especially the first two. Consider renaming the Config class.

+ "Please check the documentation."
)
# load data
config, data = load_data(folder_name)
Copy link
Member

Choose a reason for hiding this comment

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

load_data returns first config, then data. On the other hand, the order for defining PhotonData and Analysis objects is the exact opposite: first data, then config. I would make the order the same in all instances, to avoid user confusion

@@ -0,0 +1,46 @@
class Options:
"""Class with the options to analize data from suite2p and registers2p.
Copy link
Member

Choose a reason for hiding this comment

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

typo: analyze

@@ -121,3 +121,10 @@ def check_if_file_exists(self) -> bool:
True if folder exists, False otherwise
"""
return os.path.exists(self.get_path())
Copy link
Member

Choose a reason for hiding this comment

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

Use pathlib for consistency, e.g. return self.get_path().exists().
If you specifically want to check whether the path is a directory, use: return self.get_path().is_dir()

Copy link
Member

Choose a reason for hiding this comment

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

Also, if this function check for a folder (not a file), consider renaming the function

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!!

return config, data_raw


def configure(folder_name: str) -> Config:
Copy link
Member

Choose a reason for hiding this comment

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

Consider renaming this function to sth more specific, e.g. add_folder_to_config

from .options import Options


class Config:
Copy link
Member

Choose a reason for hiding this comment

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

it's confusing for me that that this class has the same name as the config.yaml, though afaik it represents something completely different.

@@ -4,7 +4,7 @@
from .parser import Parser
Copy link
Member

Choose a reason for hiding this comment

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

there is a stdlib module named parser. Consider modifying tha name of your parser module, to avoid overriding the stdlib one.

self._parser = Parser01(self.folder_name, self.config)
if self.original_config["parser"] == "Parser2pRSP":
logging.debug("Parsing folder name using Parser2pRSP")
self._parser = Parser2pRSP(self.folder_name, self.original_config)
else:
logging.debug(
Copy link
Member

Choose a reason for hiding this comment

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

should this be logging.error?

from ..objects.photon_data import PhotonData


class SpatialFrequencyTemporalFrequency:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a shorter name for this class could be SpatialFreqTemporalFreq?



class Specifications:
"""The class :class:`Config` represents the configuration of the
Copy link
Member

Choose a reason for hiding this comment

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

Remember to also change the name in the docstring



def get_specifications(folder_name: str) -> Specifications:
"""Create configuration object. It reads the configuration
Copy link
Member

Choose a reason for hiding this comment

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

edit docsting

@lauraporta lauraporta merged commit 47c1e06 into developement Dec 19, 2022
@lauraporta lauraporta deleted the scaffold branch December 19, 2022 16:34
@JoeZiminski JoeZiminski mentioned this pull request Jan 20, 2023
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.

Rewrite overall structure
3 participants