Conversation
krzywon
left a comment
krzywon
left a comment
There was a problem hiding this comment.
In addition to my inline comments, a few general notes:
- I said this in the meeting, but wanted to reiterate it - good use of atomic commits with specific commit messages
- Some python formatting would be useful. Running everything through ruff could be worthwhile.
- Relative imports can be fragile, but mostly when trying to run a python file directly. That isn't easy with Django, so it's not that huge an issue, but wanted to mention it.
- Another thing we discussed at today's meeting, but wanted to mention again, is user auth and request.user clashes. Many of the checks don't ensure the authenticated user is the data owner.
Overall, a good start to the project. I'll try to make comments every week, or as updates are made.
summerhenson
force-pushed
the
sasview-database
branch
2 times, most recently
from
6503450 to
c0c7fa0
Compare
| - id: ruff | ||
| args: [--fix, --exit-non-zero-on-fix] | ||
| files: "sasdata/fair_database/.*" | ||
| - id: ruff-format | ||
| files: "sasdata/fair_database/.*" |
There was a problem hiding this comment.
Relevant discussion: https://github.com/orgs/SasView/discussions/3171
sasdata/quantities/_units_base.py
Outdated
| def parse(unit_string: str) -> "Unit": | ||
| pass | ||
|
|
||
| def serialise_json(self): |
There was a problem hiding this comment.
Will this get extended to Child classes as well?
There was a problem hiding this comment.
I believe that was the idea, but on a second look it might make more sense to just use the string representation of a unit for serialization.
sasdata/quantities/quantity.py
Outdated
|
|
||
| QuantityType = TypeVar("QuantityType") | ||
|
|
||
| # TODO: figure out how to handle np.ndarray serialization (save as file or otherwise) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Wouldn't there be a security issue with pickle?
sasdata/fair_database/data/models.py
Outdated
|
|
||
| # data contents - maybe ManyToManyField | ||
| data_contents = models.ManyToManyField("Quantity", through="LabeledQuantity") | ||
| data_contents = models.ManyToManyField("Quantity") |
There was a problem hiding this comment.
Maybe I am missing something, but to reduce complexity, I would suggest starting with data_contents as a OneToManyField. We can reduce storage by allowing a quantity to belong to multiple data sets, but I think it will be cleaner and simpler to manage if every data set has its own data contents.
sasdata/fair_database/data/models.py
Outdated
|
|
||
| # dataset | ||
| # dataset = models.ManyToManyField(DataSet) | ||
| dataset = models.ManyToManyField(DataSet) |
There was a problem hiding this comment.
Same question here, should we for simplicity say that a data set can only belong to one session? These are things we can discuss.
| dataset = models.ForeignKey( | ||
| DataSet, on_delete=models.CASCADE, related_name="data_contents" | ||
| ) | ||
|
|
There was a problem hiding this comment.
This is the right way to connect Quantity to DataSet and better than my suggestion 👌
sasdata/fair_database/data/models.py
Outdated
| @@ -87,6 +87,10 @@ class Quantity(models.Model): | |||
|
|
|||
There was a problem hiding this comment.
I saw your comment/question about how deleting an OperationTree should work. I think cascade delete would handle this automatically if OperationTree had Quantity as an optional foreign key with on_delete=models.CASCADE, and the same for the foreign key to the parent operation.
|
If there are no more planned changes to the models, maybe we should delete the migrations and create new initial migrations based on the current state of the models. It definitely made sense to keep all intermediate migrations while the models were being developed, but this is probably a good place to start fresh. |
DrPaulSharp
force-pushed
the
refactor_24
branch
2 times, most recently
from
a16b0a6 to
e27d0cc
Compare
DrPaulSharp
force-pushed
the
refactor_24
branch
from
6e3ba34 to
c010919
Compare
DrPaulSharp
force-pushed
the
refactor_24
branch
2 times, most recently
from
d4683bd to
3df6e01
Compare
DrPaulSharp
force-pushed
the
refactor_24
branch
2 times, most recently
from
b32dbfe to
6f3b4af
Compare
rprospero
force-pushed
the
refactor_24
branch
from
490cec4 to
ee6af02
Compare
DrPaulSharp
force-pushed
the
refactor_24
branch
from
ee6af02 to
15acac0
Compare
DrPaulSharp
force-pushed
the
sasview-database
branch
from
19dd78f to
09b87ee
Compare
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
No quality gates enabled for this code.
See analysis details in CodeScene
Absence of Expected Change Pattern
- sasdata/sasdata/dataloader/readers/danse_reader.py is usually changed with: sasdata/sasdata/dataloader/readers/tiff_reader.py
Quality Gate Profile: Custom Configuration
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
This is going to need to live here now in order to avoid circular dependencies.
DrPaulSharp
force-pushed
the
sasview-database
branch
from
09b87ee to
1c4aacc
Compare
|
The latest rebase now has this changing many files in refactor_24 that weren't there before. This is not mergeable in its current state, but I'll get it over the line during the hackathon next week. |
7 participants
Database backend in progress.