feat!: Burst sampling in ResamplingHypothesesUpdater #700
Conversation
fix: type hinting fix (optional None)
jeremyshoemaker
left a comment
jeremyshoemaker
left a comment
There was a problem hiding this comment.
I know this is currently a Draft, but I started reading through it and had some comments that I didn't want to lose, so I just reviewed it anyway.
We can go through some of these when we pair later.
| class HypothesesUpdater(Protocol): | ||
| def pre_step(self) -> None: | ||
| """Runs once per step before updating the hypotheses.""" | ||
| ... |
There was a problem hiding this comment.
This is really more of a style thing, so it's not critical. The Python docs aren't helpful on this point either, because they show this style inconsistently.
The usual way to have a method or function that does nothing is to use pass and not .... That said, a method or function that has a docstring doesn't need a body at all. You only need pass when there's no docstring, because there has to be at least one statement inside a block. So these ... could just be removed completely.
| available_graph_ids.append(graph_id) | ||
| available_graph_evidences.append(np.max(self.evidence[graph_id])) | ||
|
|
||
| return available_graph_ids, np.array(available_graph_evidences) |
There was a problem hiding this comment.
I noticed this inconsistency when I loaded up this branch in PyCharm. The type of the second value in this tuple is inconsistent in this function. Up on line 706, we return a list, but here we return an np.array. I think the one on 706 should be changed to match, since lists and np.arrays behave slightly differently.
| """Return evidence for each pose on each graph (pointer).""" | ||
| return self.evidence | ||
|
|
||
| def hyp_evidences_for_object(self, object_id): |
There was a problem hiding this comment.
I don't think this is a good method to add, since it actually makes all the places it's used longer than they would be without really adding anything. It also adds in an additional function call which isn't free.
self.evidence[object_id]
# vs
self.hyp_evidences_for_object(object_id)If this method name is more descriptive, then it might make sense to change the name of self.evidence, but I don't know that it adds much.
There was a problem hiding this comment.
See my notes below in the goal_state_generation changes, since that makes things more complicated.
| self.previous_mlh = self.current_mlh | ||
| self.current_mlh = self._calculate_most_likely_hypothesis() | ||
|
|
||
| self.hypotheses_updater.post_step() |
There was a problem hiding this comment.
I know we use this pre_ and post_ pattern in a lot of places already, but this really would be better as a context manager using __enter__ and __exit__. The __exit__ method also has the advantage of being called when an exception is raised allowing action to be taken if needed.
Then the body of this method would be something like:
with self.hypotheses_updater:
thread_list = []
# The rest of the body
...| # We only displace existing hypotheses since the newly resampled hypotheses | ||
| # should not be affected by the displacement from the last sensory input. | ||
| if existing_count > 0: | ||
| if len(hypotheses_selection.maintain_ids) > 0: |
There was a problem hiding this comment.
You did this elsewhere, so let's be consistent.
| if len(hypotheses_selection.maintain_ids) > 0: | |
| if len(hypotheses_selection.maintain_ids): |
| ).inv() | ||
| object_possible_poses = self.possible_poses[self.primary_target] | ||
| if not len(object_possible_poses): | ||
| return -1 |
There was a problem hiding this comment.
Is this a sentinel value we're checking for elsewhere? Shouldn't the minimum possible pose error be 0?
|
|
||
| def __len__(self) -> int: | ||
| """Returns the total number of hypotheses in the selection.""" | ||
| return int(self._maintain_mask.size) |
There was a problem hiding this comment.
Doesn't ndarray.size already return an int?
a.size returns a standard arbitrary precision Python integer.
~ Docs for ndarray.size
| # put in range(-1, 1) | ||
| scaled_evidences[graph_id] = (scaled_evidences[graph_id] - 0.5) * 2 | ||
| if len(evidences[graph_id]): | ||
| graph_evidences = evidences[graph_id] |
There was a problem hiding this comment.
Why not change this for loop to iterate over evidences.items()? That way we don't have to look up the value multiple times, but we still have both the key and the value.
| max_evidence = max(max_evidence, np.max(graph_evidences)) | ||
|
|
||
| for graph_id in evidences.keys(): | ||
| graph_evidences = evidences[graph_id] |
There was a problem hiding this comment.
Same thing. If we iterate over evidence.items() we get the graph_id and the graph_evidences at the same time.
| self._resampling_multiplier(rlm) | ||
| self._resampling_multiplier_maximum(rlm, pose_defined=True) | ||
| self._resampling_multiplier_maximum(rlm, pose_defined=False) |
There was a problem hiding this comment.
You mentioned that the only reason we're doing all of these in a single test is because of concerns about setup costs for the experiment.
I think we should refactor this class to make these individual methods into test_ methods, and move the contents of setUp and get_pretrained_resampling_lm to the setUpClass classmethod. That way, the experiment gets set up only once for this whole class, and then each individual test can test what they need to.
This will shake out any unintended dependencies between these tests, and make it easier to run them in isolation if we're ever debugging a specific issue.
There was a problem hiding this comment.
|
Thanks @jeremyshoemaker, I addressed the comments we discussed, except for the additional tests. I'll be working on those next. |
|
@jeremyshoemaker, I've added a few unit tests that specifically cover the burst sampling behavior. We can discuss if we need to add more when we pair next week. |
ramyamounir
added
rfc:implementation
2 participants
This is the second PR in the implementation project of the burst sampling hypothesis updater. The PR mainly introduces two main features:
HypothesesSelectioncontainer class in introduced to refer to a subset of hypotheses between different parts of the code.Additional, more detailed, context can be found in this PR description
Benchmarks
We will not be updating any benchmarks (other than unsupervised inference) because this PR doesn't set
ResamplingHypothesesUpdater. That said, I still ran the benchmarks to make sure we are getting the same benefits we saw in the feature repo. Results of the benchmark runs are tagged withPR#700base_config_10distinctobj_dist_agent
base_config_10distinctobj_surf_agent
randrot_noise_10distinctobj_dist_agent
randrot_noise_10distinctobj_dist_on_distm
randrot_noise_10distinctobj_surf_agent
randrot_10distinctobj_surf_agent
randrot_noise_10distinctobj_5lms_dist_agent
base_10simobj_surf_agent
randrot_noise_10simobj_dist_agent
randrot_noise_10simobj_surf_agent
randomrot_rawnoise_10distinctobj_surf_agent
base_10multi_distinctobj_dist_agent
base_77obj_dist_agent
base_77obj_surf_agent
randrot_noise_77obj_dist_agent
randrot_noise_77obj_surf_agent
randrot_noise_77obj_5lms_dist_agent
unsupervised_inference_distinctobj_dist_agent
unsupervised_inference_distinctobj_surf_agent