Sasview database #106
Sasview database #106
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
| operation_tree = models.OneToOneField( | ||
| "OperationTree", blank=True, null=True, on_delete=models.SET_NULL | ||
| ) | ||
|
|
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. |
This is to make it easier to call this outside of the reader dialog GUI.
This seems to be a cleaner solution.
So that we don't have to initialise it when calling it.
It doesn't :(
Not named quantities.
Truncate them for now.
Update the ASCII reader to import raw metadata
Beginning Modelling requirements
Applies automatic fixes from the default ruff ruleset
This is a bit of a bodge for now, but a more permanent fix will be applied in the unit test refactor.
Also Ruff format.
Fix ASCII reader column guesses
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
Database backend in progress.