gQSPSim / gQSPSim_public

BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

RootDirectory #3

Open simkaryote opened 1 year ago

simkaryote commented 1 year ago

Path handling in the code continues to be problematic. It is holding back the addition of tests. I will not have time to fix this therefore tests will continue to fail until it is fixed. Other tests will be added once this issue is addressed.

simkaryote commented 1 year ago

Giving more detail here on the specifics.

OptimizationData does not update its paths when the RootDirectory is changed on a Session. I don't know if there are other objects that need the same fix.

In general, the approach used for paths in the +QSP code (and probably elsewhere) could be improved such that not every new object added needs to be aware of the relationship with Session's RootDirectory.

As I have said before, the concept of a project would be beneficial. If the need to aggregate paths outside the RootDirectory is a requirement that could be accomplished also without causing every path to have a dependency on RootDirectory (or where the files where last time someone saved them.)

feigelman commented 1 year ago

Can you help me better understand the issue?

The QSP.OptimizationData class doesn't store any paths as properties. Where is it supposed to update?

This class does inherit from QSP.abstract.BaseProps, which defines the methods for FilePath and RelativeFilePath. Is it that the relative and/or full file paths are not being constructed correctly when a session's RootDirectory is changed?

The paradigm we followed is that at time of absolute file path selection through UI, the path relative to root is computed and stored. If the RootDirectory is later changed, the new abs file path is constructed based on the relative path. That seems entirely reasonable to me. Can you clarify your comment that objects should not need to be aware of their relationship to the RootDirectory, i.e their relative file path?

Also, which paths need to be tracked (aggregated?) outside of the Root Directory? I think it is just plugins which are allowed to live outside the Root Directory, since those may be shared across projects.

simkaryote commented 1 year ago

The various entries of session.Settings (e.g., OptimizationData) have absolute paths stored in them. That is a problem in general for how one user sends a configuration/setup (call it a project if you'd like) to another. For a long time we have had the issue that these objects store paths that will not exist for other users.

This problem affects running tests and is holding that effort back.

I would suggest removing absolute paths from all objects and ALWAYS composing a path using the RootDirectory property. Note that trying to circumvent the problem by setting the RootDirectory on a session does not fully address the issue because not all objects get updated with a new absolute path. However, I stress that even if setting the RootDirectory did update all paths it is still a poor design decision to implement the concept of a project this way.

You have expressed the need to have a "project" reference files not under the same directory (no RootDirectory). If that really is a use case then I would suggest first implementing the concept of a project (all under one directory). Get that working cleanly and then add the concept of "external" files that are referenced by an auxiliary property with the path used. I would also make it so there is a mechanism to pull those external references into a project directory in order to enable sharing with others. Or at least a way to list the files that are "external" such that a user can decide what to do. I would also provided a way to remove all external references upon export so that these paths don't become public and break others.

I hope this helps explain the issue.

For a concrete example, take a look at the following test: tCLI/loadSession. This test fails because it looks for a Data_mean.xlsx in the wrong place even though the RootDirectory has been setup correctly.

This is the reason the tests are failing in GitHub.