QDax icon indicating copy to clipboard operation
QDax copied to clipboard

Add Multi-Emitter

Open Lookatator opened this issue 2 years ago • 1 comments

Add a base Multi-Emitter implementation of a batch of Emitters.

TODOs:

  • [ ] Tests
  • [ ] Documentation

Lookatator avatar Sep 09 '22 10:09 Lookatator

Codecov Report

Merging #90 (1db8b2f) into develop (46e82b6) will increase coverage by 0.07%. The diff coverage is 93.75%.

@@             Coverage Diff             @@
##           develop      #90      +/-   ##
===========================================
+ Coverage    90.69%   90.77%   +0.07%     
===========================================
  Files           84       86       +2     
  Lines         4677     4789     +112     
===========================================
+ Hits          4242     4347     +105     
- Misses         435      442       +7     
Impacted Files Coverage Δ
qdax/core/emitters/cma_mega_emitter.py 98.63% <66.66%> (-1.37%) :arrow_down:
qdax/core/emitters/omg_mega_emitter.py 97.95% <66.66%> (-2.05%) :arrow_down:
qdax/core/emitters/pga_me_emitter.py 99.28% <66.66%> (-0.72%) :arrow_down:
qdax/core/emitters/emitter.py 90.90% <75.00%> (-3.54%) :arrow_down:
qdax/core/emitters/multi_emitter.py 96.55% <96.55%> (ø)
...ests/core_test/emitters_test/multi_emitter_test.py 97.36% <97.36%> (ø)
qdax/core/emitters/standard_emitters.py 96.96% <100.00%> (+0.30%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov-commenter avatar Sep 09 '22 10:09 codecov-commenter

* is it possible to make the lambda function of the update_state a bit less verbose?

Yup! Just did it in the last commit!

* what do you think about computing the start/end indexes only once in the **init** function and storing them as attributes of the class; and use them in state_update. Instead of recomputing them every time. Would potentially improve clarity. What do you think?

I am unsure if they are recomputed everytime (due to the jit...) But indeed, it definitely improves clarity. I just pushed the changes :)

Other then that, I am okay with the batch_size property, I think it is important now that we are starting to work with meta-emitters.

Sounds good! Thanks for the feedback!

Lookatator avatar Nov 21 '22 15:11 Lookatator