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

Do we want id associated with data rather than the DO itself? #224

Closed
Tracked by #143
bobleesj opened this issue Dec 13, 2024 · 3 comments
Closed
Tracked by #143

Do we want id associated with data rather than the DO itself? #224

bobleesj opened this issue Dec 13, 2024 · 3 comments

Comments

@bobleesj
Copy link
Contributor

bobleesj commented Dec 13, 2024

Continuing our discussion from #216. @sbillinge wrote:


@bobleesj @yucongalicechen I merged this but now I am having second thoughts. Do we want the id associated with the data rather than the DO itself? In that case we should mint it in load_data and not in the constructor.

More discussion: this would go away if we don't allow an empty instance where data are loaded later. I started leaning that way because I couldn't think of a UC where we would really need that, though one would be if someone wants to load directly from a file to the DO. We could also allow that as an option in the constructor too. Can you have a think about it?

If we do mint the id on the load data function, all of our setters will have to have it insterted too.

Summary: maybe the cleanest is to mint it in the constructor as we do now but remove the ability to load data outside of the constructor (and make that method private). But aloe an option to instantiate from file by giving a file path instead of a set of arrays.

What do you think? I think I like that...

Originally posted by @sbillinge in #216 (comment)


@bobleesj
Copy link
Contributor Author

bobleesj commented Dec 13, 2024

maybe the cleanest is to mint it in the constructor as we do now but remove the ability to load data outside of the constructor

I find that id associated with DO feels more natural since an object certainty has an unique identifier like a barcode.

More discussion: this would go away if we don't allow an empty instance where data are loaded later. I started leaning that way because I couldn't think of a UC where we would really need that, though one would be

I find that having an empty instance for DiffractionClass requires us to do a bunch of is not None stuff and convert the none parameters into string or an empty array. I find this a source of error and confusion.

Unless we have a very specific use case for this empty instance. I can't think of one. In my mind, why would a person create this object without data?

@sbillinge my thoughts above.

@sbillinge
Copy link
Contributor

Yes, I feel this is the right approach. Let's do it.

@bobleesj
Copy link
Contributor Author

Closing this issue - since we have a new issue made above.

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

No branches or pull requests

2 participants