Add Context - resolves #1 #4

jasonasher · 2024-07-19T19:25:24Z

This imports the code from eosim for Context.

src/context.rs

k88hudson-cfa · 2024-07-22T14:00:18Z

src/context.rs

+ callbacks_to_return
+ }
+
+ pub fn release_event<E: Copy + 'static>(&mut self, event: E) {


Is the event emitter stuff new? We may way to split this out

Pulled out in latest commit

src/context.rs

bbbruce · 2024-07-22T14:09:41Z

src/context.rs

+ pub fn get_next_timed_plan(&mut self) -> Option<TimedPlan> {
+ loop {
+ let next_timed_plan = self.queue.pop();
+ match next_timed_plan {


Can .map be used here?

k88hudson-cfa

Can you rebase on main (which should remove the precommit changes) and also split out the event emitter stuff for now? I think after discussing it would be good to merge that separately.

Will do a more thorough pass after that's done

jasonasher · 2024-07-30T23:08:30Z

src/context.rs

+pub use define_plugin;
+
+pub struct PlanId {
+ pub id: u64,


Maybe this should not have pub visibility? People using Context should only work with PlanIds that are returned and not construct them directly.

k88hudson-cfa · 2024-07-30T22:32:03Z

src/context.rs

+
+use derivative::Derivative;
+
+pub trait Plugin: Any {


As per discussion let's call this DataPlugin

k88hudson-cfa · 2024-07-30T22:34:28Z

src/context.rs

+}
+
+#[macro_export]
+macro_rules! define_plugin {


This is also has to get renamed (and other usages)

k88hudson-cfa · 2024-07-30T22:40:01Z

src/context.rs

+pub trait Plugin: Any {
+ type DataContainer;
+
+ fn get_data_container() -> Self::DataContainer;


I think this would make more sense if it was called create_data_container()

k88hudson-cfa · 2024-07-30T22:46:38Z

src/context.rs

+}
+pub use define_plugin;
+
+pub struct PlanId {


Can you put all this plan stuff in a plan.rs file?

k88hudson-cfa · 2024-07-30T23:07:23Z

src/context.rs

+pub struct TimedPlan {
+ pub time: f64,
+ plan_id: u64,
+ #[derivative(PartialEq = "ignore", Debug = "ignore")]


Maybe add a comment about why this is needed

k88hudson-cfa · 2024-07-31T00:16:56Z

src/context.rs

+ }
+
+ pub fn cancel_plan(&mut self, id: PlanId) {
+ self.invalid_set.insert(id.id);


As we discussed live, this should be structured in some way that it will panic or at least warn if you try to cancel a plan that has already run. I think you don't want to be in a scenario where there's a bug in your logic where you do the cancel too late but it just sort of fails silently.

Yes, good point. I implemented this in one relatively easy way and added tests.

ekr-cfa · 2024-08-01T13:27:32Z

src/plan.rs

+};
+
+pub struct Id {
+ id: usize,


Why is this usize? I would probably use u64.

ekr-cfa · 2024-08-01T13:29:01Z

src/plan.rs

+}
+
+#[derive(PartialEq, Debug)]
+pub struct Record {


Suggested change

pub struct Record {

struct Record {

I would also consider calling this QueueEntry or Entry

ekr-cfa · 2024-08-01T13:39:53Z

src/plan.rs

+ if time_ordering == Ordering::Equal {
+ // Break time ties in order of plan id
+ self.id.cmp(&other.id).reverse()
+ } else {
+ time_ordering
+ }
+ }


I think this would be cleaner as:

Suggested change

if time_ordering == Ordering::Equal {

// Break time ties in order of plan id

self.id.cmp(&other.id).reverse()

} else {

time_ordering

}

}

match time_ordering {

Ordering::Equal => self.id.cmp(&other.id).reverse(),

_ => time_ordering

}

ekr-cfa · 2024-08-01T13:42:47Z

src/plan.rs

+ /// This function panics if you cancel a plan which has already
+ /// been cancelled or executed.
+ pub fn cancel_plan(&mut self, id: &Id) {
+ self.data_map.remove(&id.id).expect("Plan does not exist");


I would add a comment, as in:

Delete the plan from the map, but leave it in the queue. It will be skipped when we get to the deadline.

ekr-cfa · 2024-08-01T13:43:02Z

src/plan.rs

@@ -0,0 +1,139 @@
+use std::{


I think this could use a block comment at the top describing the architecture.

ekr-cfa · 2024-08-01T13:43:32Z

src/plan.rs

+ loop {
+ match self.queue.pop() {
+ Some(plan_record) => {
+ if let Some(data) = self.data_map.remove(&plan_record.id) {


I would also add a comment here to explain what is happening.

ekr-cfa · 2024-08-01T13:43:47Z

src/plan.rs

+ pub fn get_next_plan(&mut self) -> Option<Plan<T>> {
+ loop {
+ match self.queue.pop() {
+ Some(plan_record) => {


If you take my advice above, I would call this entry

ekr-cfa

I think this is basically ready, except for my comment about get_data_container() which I think requires either a change or an argument about why I'm wrong.

Either way, we should have a test that exercises calling get_data_container() when one doesn't exist.

ekr-cfa · 2024-08-02T23:39:02Z

src/context.rs

+/// The simulation also has an immediate callback queue to allow for the
+/// accumulation of side effects of mutations by the current controlling module.


Suggested change

/// The simulation also has an immediate callback queue to allow for the

/// accumulation of side effects of mutations by the current controlling module.

/// The simulation also has a separate callback mechanism. Callbacks

/// fire before the next timed event (even if it is scheduled for the

/// current time. This allows modules to schedule actions for immediate

/// execution but outside of the current iteration of the event loop.

ekr-cfa · 2024-08-02T23:41:45Z

src/context.rs

+ ///
+ /// Returns a reference to the data container if it exists or else `None`
+ #[must_use]
+ pub fn get_data_container<T: DataPlugin>(&self) -> Option<&T::DataContainer> {


I'm not so sure this is the right design choice. Why make readers deal with the possibility that this is empty rather than returning an empty container?

The idea is that we want this method to return a reference to the data container owned by the Context if it exists. If it doesn't there isn't a way to return a reference as nothing would own the data container. Circumventing this would entail interior mutability which I think for now we want to avoid.

ekr-cfa · 2024-08-02T23:42:15Z

src/context.rs

+ }
+}
+
+impl Default for Context {


Nit: I would put this up near new()

I think it's idiomatic to follow the struct with its impl before doing other traits, so I don't think there is a way to move this up more and keep with that.

ekr-cfa · 2024-08-02T23:42:39Z

src/context.rs

+ }
+}
+
+type Callback = dyn FnOnce(&mut Context);


Nit: I would put this earlier because we depend on it.

ekr-cfa · 2024-08-02T23:43:06Z

src/context.rs

+ }
+ };
+}
+pub use define_data_plugin;


I would either put this stuff earlier or alternately, in a different file.

I wanted to define Context first as it was the main focus of this source file. It seemed natural to have the DataPlugin defined after that together with its construction macro.

I think there is a philosophical question here. In general, I think that things should appear before things that depend on them, or in some separate location and be imported in. In this case, neither is true. The question is whether it's possible to infer what DataPlugin does from the code that uses it, and I am skeptical of that.

ekr-cfa · 2024-08-02T23:53:44Z

src/context.rs

+mod tests {
+ use super::*;
+
+ define_data_plugin!(ComponentA, Vec<u32>, vec![]);


These are really thorough tests. This is approximately how I would do it.

Thanks, I worked hard to get those right.

ekr-cfa · 2024-08-02T23:54:23Z

src/lib.rs

+//! * A disease progression manager that transitions infected people through
+//! stages of disease until recovery.
+//! * A transmission manager that models the process of an infected
+//! person trying to infect susceptible people in the population.


This doc is fine, but I wouldn't put it in lib.rs. I would just put it in README.md or something.

I put it here so it would show up in the generated Rust docs for the library. Will remove if you feel strongly.

ekr-cfa · 2024-08-02T23:54:59Z

src/plan.rs

+
+/// A priority queue that stores arbitrary data sorted by time
+///
+/// Items of type T are stored in order by `f64` time and called `Plan<T>`.


Suggested change

/// Items of type T are stored in order by `f64` time and called `Plan<T>`.

/// Items of type `T` are stored in order by `f64` time and called `Plan<T>`.

ekr-cfa · 2024-08-02T23:55:08Z

src/plan.rs

+/// Items of type T are stored in order by `f64` time and called `Plan<T>`.
+/// When plans are created they are sequentially assigned an `Id` that is a
+/// wrapped `u64`. If two plans are scheduled for the same time the plan that is
+/// scheduled first (i.e. that has the lowest id) is placed earlier.


Suggested change

/// scheduled first (i.e. that has the lowest id) is placed earlier.

/// scheduled first (i.e., that has the lowest id) is placed earlier.

ekr-cfa · 2024-08-02T23:59:36Z

src/plan.rs

+ assert_eq!(next_plan.time, 3.0);
+ assert_eq!(next_plan.data, 3);
+
+ assert!(plan_queue.get_next_plan().is_none());


I'd like to see a test that demonstrates that you can add a plan once you started servicing the queue, not just all at once.

ekr-cfa · 2024-08-03T21:29:30Z

src/plan.rs

+ assert_eq!(next_plan.time, 1.0);
+ assert_eq!(next_plan.data, 1);
+
+ plan_queue.add_plan(3.0, 3);


I would consider adding it with 1.5 to demonstrate that sort works properly.

ekr-cfa

LGTM, but I added one comment below.

k88hudson-cfa

🚢 it

jasonasher requested a review from k88hudson-cfa July 19, 2024 19:25

jasonasher linked an issue Jul 19, 2024 that may be closed by this pull request

Add Context #1

Closed

k88hudson-cfa reviewed Jul 22, 2024

View reviewed changes

bbbruce reviewed Jul 22, 2024

View reviewed changes

k88hudson-cfa requested changes Jul 28, 2024

View reviewed changes

jasonasher force-pushed the initial-context branch from 324171f to b1620b3 Compare July 28, 2024 17:37

jasonasher requested a review from k88hudson-cfa July 28, 2024 17:38

k88hudson-cfa mentioned this pull request Jul 29, 2024

Add event emitter / handler functionality #8

Closed

jasonasher commented Jul 30, 2024

View reviewed changes

k88hudson-cfa requested changes Jul 30, 2024

View reviewed changes

k88hudson-cfa reviewed Jul 31, 2024

View reviewed changes

jasonasher requested a review from k88hudson-cfa July 31, 2024 04:58

jasonasher and others added 9 commits July 31, 2024 23:09

Import context from eosim

a35f41d

Remove event emission and handle comments from review

19f6e6f

Clean up main loop

e24d5fc

Clean up even more

a814d82

Separate plan code from context

ce2bb7e

Rename plugin concept

b0b63f3

Add tests for plan queue and handle invalid cancellation

bf06af8

Refactor context tests so that they test individual behaviors.

f021aee

Format

f2875b3

jasonasher force-pushed the initial-context branch from 291f21a to f2875b3 Compare August 1, 2024 03:09

ekr and others added 5 commits July 31, 2024 20:21

Fix the first clippy errors

d4ca244

Clippy fixes

8117ed7

Fmt

dd4a355

Fix clippy

d78dfb7

Update data container code and handle pedantic clippy

d165d0e

ekr-cfa requested changes Aug 1, 2024

View reviewed changes

jasonasher added 2 commits August 1, 2024 23:42

Document plan code and split up tests

8010a4d

Add more docs

724667a

ekr-cfa requested changes Aug 3, 2024

View reviewed changes

Address comments from review and add tests

70a6bfb

jasonasher force-pushed the initial-context branch from 675915f to 70a6bfb Compare August 3, 2024 03:40

jasonasher requested a review from ekr-cfa August 3, 2024 03:45

ekr-cfa reviewed Aug 3, 2024

View reviewed changes

ekr-cfa approved these changes Aug 3, 2024

View reviewed changes

Modify test based on review

327c410

k88hudson-cfa approved these changes Aug 4, 2024

View reviewed changes

jasonasher merged commit a8c4e9d into main Aug 4, 2024
4 checks passed

		/// The simulation also has an immediate callback queue to allow for the
		/// accumulation of side effects of mutations by the current controlling module.

-/// The simulation also has an immediate callback queue to allow for the
-/// accumulation of side effects of mutations by the current controlling module.
+/// The simulation also has a separate callback mechanism. Callbacks
+/// fire before the next timed event (even if it is scheduled for the
+/// current time. This allows modules to schedule actions for immediate
+/// execution but outside of the current iteration of the event loop.

	/// Items of type T are stored in order by `f64` time and called `Plan<T>`.
	/// Items of type `T` are stored in order by `f64` time and called `Plan<T>`.

	/// scheduled first (i.e. that has the lowest id) is placed earlier.
	/// scheduled first (i.e., that has the lowest id) is placed earlier.

Add Context - resolves #1 #4

Add Context - resolves #1 #4

Conversation

jasonasher commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k88hudson-cfa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonasher Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekr-cfa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonasher Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekr-cfa left a comment

Choose a reason for hiding this comment

k88hudson-cfa left a comment

Choose a reason for hiding this comment

jasonasher Jul 31, 2024 •

edited

Loading

jasonasher Aug 3, 2024 •

edited

Loading