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

Improve dummy-blocks for better test data #736

Merged
merged 7 commits into from
Mar 20, 2023

Conversation

owi92
Copy link
Member

@owi92 owi92 commented Mar 13, 2023

This is adds a couple of functions that insert various types of dummy blocks (series, text, title, video).

I'm sure there are a lot of things that can be improved upon, as I am still pretty new to rust. Some things might look weird to seasoned rust developers, so I left some comments trying to explain what the code is meant to do/what I think it does.

Closes #601 when eventually completed.

@github-actions github-actions bot temporarily deployed to test-deployment-pr736 March 13, 2023 12:01 Destroyed
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, better test data!
I've left some inline comments, sometimes explaining Rust stuff. But overall it's in a decent shape!

I think the resulting blocks could be improved a bit. I think ideally, in my head, I would like the following. Note: lots of these things can be done later, so it's fine for you to concentrate on a subset of this first, e.g. first fixing/adjusting the parts you already have.

  • (A) First: add series blocks to realms with matching names, as you do now. However, for those realms I would:
    • Only consider realms that have no blocks yet at all.
    • Not add a dummy text block
    • Make the realm name be derived from the series block.¹
    • Make the series block to NOT show the title, BUT to show the description.²
    • (All these measures will lead to a kind of realm which I expect to be most common in real life)
  • All realms and series touched in the previous step are now "out of the equation": those realms are not further edited and those series are not used anywhere else.
  • Further, remove a few more series (like 10% of the remaining ones?) and events from the list of assignable series/events. That way we make sure that there will be some series and events that are not used in any realm.
  • (B) For all realms that have no blocks yet:
    • Randomly³, in most cases add one text block, in a few cases add two, otherwise add none. (Or even more than two, exact distribution up to you)
    • Randomly, with a probability of remaining_series/remaining_realms⁴, add a series block of a random series. Do not remove the series from the pool so that some will (statistically) be used in more than one realm.
    • Randomly³, add none or a small number of videos blocks (exact distribution up to you). Similarly, I would not remove the used videos from the pool to have a chance one is used in more than one realm.
    • Shuffle the block order⁵

¹ This is achieved by setting the name_from_block column to the block ID and set name to null. It's a bit tricky to implement this in this file. You can most certainly do this step last, even as a separate commit if you like. I would add a name_from_block: Option<usize> field to Realm that you annotate with #[serde(default)] (such that the field does not have to be specified in the YAML file). Next, Block::insert needs to be changed to return the ID of the inserted block (an i64). You can use the SQL syntax returning id for that. And instead of db.execute() you have to do db.query_one then. Finally, add some logic in insert_realm to use name_from_block and the returned id from block.insert() to conditionally execute another SQL query setting the columns as described.

² Also something you can do as separate commits. To be able to specify whether title and/or description should be shown, I would change the Series variant in the Block enum from Series(SeriesBlock) to Series { show_title: bool, show_description: bool, series: SeriesBlock }. This unfortunately requires you to also manually adjust realms.yaml, but luckily only the three series blocks in the root realm. Instead of series:\n by_uuid: 123 -> series:\n show_title: true\n show_title: false\n series:\n by_uuid: 123.

³ For these "weighted" random selections, take a look at WeightedIndex in the rand library.

⁴ Getting the value for remaining_realms is not trivial maybe. But once you have the generic for_all_realms, I think you can just count them fairly easily?

⁵ There is a utlity in rand for that: shuffle.

I think this should be doable with three calls to for_all_realms (one for (A), one for (B), one for calculating remaining_realms, see ⁴). And I would think that results in quite nice and realistic test data! Also, the keeps the previous behavior that --dummy-blocks only affects realms with no blocks defined in the YAML file at all.

backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
This is adds a couple of functions that insert various types of
dummy blocks (series, text, title, video).
@github-actions github-actions bot temporarily deployed to test-deployment-pr736 March 16, 2023 17:25 Destroyed
@owi92 owi92 marked this pull request as ready for review March 16, 2023 17:29
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just small stuff left, mostly about comments!

backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an oversight

backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to test-deployment-pr736 March 16, 2023 17:57 Destroyed
This adds a wrapper function which contains logic for recursively calling
an inner function to do operations on all realms.
The actual blocks are then added in two calls to this wrapper:
First, realms without any block get a matching series, if that exists.
Then, the remaining series are distributed over any other yet blockless realm,
along with additional text and video blocks.
@github-actions github-actions bot temporarily deployed to test-deployment-pr736 March 16, 2023 19:31 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr736 March 16, 2023 20:21 Destroyed
backend/src/cmd/import_realm_tree.rs Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Show resolved Hide resolved
backend/src/cmd/import_realm_tree.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to test-deployment-pr736 March 20, 2023 08:25 Destroyed
@LukasKalbertodt
Copy link
Member

Admin UI CI check fails as the API is different on master. This would be solved by rebasing but since this is a mixed-author PR and I don't want to touch Oles commits, lets just merge. There is no real problem anyway.

@LukasKalbertodt LukasKalbertodt merged commit 64a3000 into elan-ev:master Mar 20, 2023
@LukasKalbertodt LukasKalbertodt added the changelog:dev Changes primarily for developers label May 15, 2023
@owi92 owi92 deleted the improve-dummy-blocks branch March 4, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:dev Changes primarily for developers kind:improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve --dummy-blocks of import-realm-tree to create more realistic conditions
2 participants