Skip to content

refactor rows with metadata API #141

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

Merged
merged 1 commit into from
Jul 21, 2021
Merged

Conversation

molpopgen
Copy link
Member

@molpopgen molpopgen commented Jul 21, 2021

See #141 for background.

@molpopgen
Copy link
Member Author

@MomoLangenstein -- this PR is a quick version of what you suggested in #129 re: Option<Metadata Trait>.

Now that I've undone the Option, I believe I've rediscovered my original thinking:

  1. Metadata is optional per-row.
  2. rust doesn't allow function overloads, which is normally fine but annoying here.
  3. If the metadata, by which we mean some rust type containing rich information about important things like mutations or individuals, is large, there may be times we don't want to write it out. For example, we may decide not to write it out for mutations not affecting fitness in a forward sim.
  4. So, rather than have the client code worry about which of 2 functions to call, I decided to make them worry about Some(&foo) vs None.

But, since this change is API-breaking, perhaps even more than the strong row ID types, I figure now is a good time to discuss it, so that we can have as many breaking changes as possible in one release.

@molpopgen molpopgen marked this pull request as draft July 21, 2021 19:38
@juntyr
Copy link
Contributor

juntyr commented Jul 21, 2021

Now that you’ve described a use-case, I think you could keep the exposed version taking an option. This explicit version could either be renamed to with_optional_metadata OR keep its current name (so nothing breaks) and the new function is called with_some_metadata.

@juntyr
Copy link
Contributor

juntyr commented Jul 21, 2021

What do you think about replacing the &dyn (I.e. dynamic dispatch) with a generic parameter?

@molpopgen
Copy link
Member Author

The dyn is an interesting question! So far, I've seen little evidence of a performance hit, but I'll admit that I haven't looked hard.

There is a use case for dyn, but it may be that it requires someone doing something a bit crazy:

  • Your simulation has many types that implement some concept/trait of Individual. (Think 1 trait per species, maybe?)
  • These are stored as Vec<Box<dyn Individual>> for the entire simulation.
  • Each individual implements MetadataRoundTrip.

Since I've never tried this, and am only just reading the not free book, I'm unsure if this just wouldn't work as generics, due to type erasure. Or, it may work but compile loads of versions of the add_foo function. Coming from many years of C++, my intuition leans towards the type erasure problem coming up, but I could be wrong.

@juntyr
Copy link
Contributor

juntyr commented Jul 21, 2021

The dyn is an interesting question! So far, I've seen little evidence of a performance hit, but I'll admit that I haven't looked hard.

In most use cases, there won’t be any performance difference at all. Unless you throw a branch prediction benchmark at this, there should be no difference at all.

There is a use case for dyn, but it may be that it requires someone doing something a bit crazy:

  • Your simulation has many types that implement some concept/trait of Individual. (Think 1 trait per species, maybe?)
  • These are stored as Vec<Box<dyn Individual>> for the entire simulation.
  • Each individual implements MetadataRoundTrip.

Since I've never tried this, and am only just reading the not free book, I'm unsure if this just wouldn't work as generics, due to type erasure. Or, it may work but compile loads of versions of the add_foo function. Coming from many years of C++, my intuition leans towards the type erasure problem coming up, but I could be wrong.

You are right, it would compile one version per type it is specialised for (though the compiler implements optimisations to specialise as little as possible).

By using dyn, you support every use case directly. If you use generics, then someone with this special use case would have to write their own tiny wrapper struct which contains the dyn and does the dispatch inside.

@molpopgen
Copy link
Member Author

You are right, it would compile one version per type it is specialised for (though the compiler implements optimisations to specialise as little as possible).

By using dyn, you support every use case directly. If you use generics, then someone with this special use case would have to write their own tiny wrapper struct which contains the dyn and does the dispatch inside.

Cool, thanks! So, maybe I'll keep the existing API (to not break things), add a _with_some_ like you suggest, and leave it there. I'd like to allow for outside-the-box-and-a-bit-out-there designs to work with little pain.

@molpopgen molpopgen force-pushed the refactor_rows_with_metadata_API branch from ee2c68f to afb4696 Compare July 21, 2021 20:34
@juntyr
Copy link
Contributor

juntyr commented Jul 21, 2021

We’ll also need to fix the individuals functions here

@molpopgen
Copy link
Member Author

We’ll also need to fix the individuals functions here

Yeah, that's happening once I merge that other PR & rebase here.

@molpopgen molpopgen force-pushed the refactor_rows_with_metadata_API branch from f801234 to 27bf110 Compare July 21, 2021 21:12
add rows with Some (non-Option-al) metadata.

Closes #131
@molpopgen molpopgen force-pushed the refactor_rows_with_metadata_API branch from 27bf110 to 58c7b6b Compare July 21, 2021 22:40
@molpopgen molpopgen marked this pull request as ready for review July 21, 2021 22:47
@molpopgen molpopgen merged commit 7a716bb into main Jul 21, 2021
@molpopgen molpopgen deleted the refactor_rows_with_metadata_API branch July 21, 2021 22:48
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.

2 participants