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

Add QueueManager (Juan Chavez's WAVES project) #414

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

emilydolson
Copy link
Collaborator

Finally had a chance to put the necessary finishing touches on this! It is a class that makes a web interface for queuing up multiple runs on a web GUI. Useful for educational purposes (since students can actually collect data in the web version of a program) and for writing simple models to hand off to non-coders to analyze.

I still need to write some tests for it before this is ready for merge, but I updated the SpatialCoop2017 demo to use it (since that was the initial inspiration) and it's working fine.

@emilydolson emilydolson marked this pull request as ready for review March 1, 2021 08:19
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #414 (00b0b69) into master (ea87bef) will increase coverage by 1.00%.
The diff coverage is 97.34%.

❗ Current head 00b0b69 differs from pull request most recent head 32121ab. Consider uploading reports for the commit 32121ab to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
+ Coverage   82.02%   83.03%   +1.00%     
==========================================
  Files         294      297       +3     
  Lines       33200    33596     +396     
==========================================
+ Hits        27233    27897     +664     
+ Misses       5967     5699     -268     
Impacted Files Coverage Δ
include/emp/prefab/QueueManager.hpp 94.11% <94.11%> (ø)
include/emp/config/SettingConfig.hpp 84.98% <95.74%> (+84.98%) ⬆️
include/emp/datastructs/map_utils.hpp 86.95% <100.00%> (ø)
include/emp/datastructs/vector_utils.hpp 100.00% <100.00%> (ø)
tests/config/SettingConfig.cpp 100.00% <100.00%> (ø)
tests/web/QueueManager.cpp 100.00% <100.00%> (ø)
include/emp/Evolve/World.hpp 96.80% <0.00%> (-0.32%) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea87bef...32121ab. Read the comment docs.

@emilydolson
Copy link
Collaborator Author

emilydolson commented Mar 1, 2021

Okay, tests written, ready for review! Since this is web code it won't show up in the coverage :(.

Does prefab seem like the right place for this?

@emilydolson
Copy link
Collaborator Author

And test coverage is now sufficient! The process of writing tests for SettingConfig revealed an obscure issue where in certain cases if you include both map_utils.h and vector_util.h and then try to call Has on a vector, the compiler will try to use the map_utils.h version instead. So this pull request now includes a fix for that as well.

Copy link
Member

@mmore500 mmore500 left a comment

Choose a reason for hiding this comment

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

Could you make a pass to undo the 4-indent to 2-indent whitespace changes you made? I bet your editor maybe just changed all that I assume it's not intentional? It's making telling what changed in the diffs really hard and it looks like 2-indent is being used everywhere else in Empirical. Ideally, we would fixup the git history and remove these changes so that git blame would be more useful on these files in the future but I don't think that's really worth the hassle in this case. We could just squash commit, which would be easy but then we'd lose all of the history of how it was put together.

Ideally, we'd might run everything through a formatter and be done with it but we haven't set that up :/

demos/SpatialCoop2017/source/web/SimplePDWorld-web.cpp Outdated Show resolved Hide resolved
@emilydolson
Copy link
Collaborator Author

Oof, I'm not sure what happened to the formatting, but it was worse than it looked. My guess is that Juan's editor did some sort of autoformatting and mine just went along with it when I was doing this final pass. And because we're not using a real formatting standard for Empirical there wasn't a way to auto-fix it. So what I'm saying is that I would absolutely be in favor of running everything through clang-tidy or something on commit :). Also might be worth explicitly discussing in an early WAVES enrichment this year.

Anyway, it's fixed now.

@emilydolson emilydolson requested a review from mmore500 March 18, 2021 21:42
Copy link
Member

@mmore500 mmore500 left a comment

Choose a reason for hiding this comment

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

A few minor suggestions, but looks great!

include/emp/config/SettingConfig.hpp Show resolved Hide resolved
return result;
}

/// Retrieves all of the setting names in config and places into std::vector
Copy link
Member

Choose a reason for hiding this comment

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

don't think this docstring is quite right

include/emp/prefab/QueueManager.hpp Show resolved Hide resolved
* tool was written by Juan Chavez as part of WAVES 2020)
*/

#pragma once
Copy link
Member

Choose a reason for hiding this comment

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

Switch this out for #ifndef/#define style header guards, I think we're trying to stay /entirely/ standards-compliant

include/emp/prefab/QueueManager.hpp Show resolved Hide resolved
display_div << queue_button;
}

/// Adds new metric to table
Copy link
Member

Choose a reason for hiding this comment

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

Missing . at end

/// a lambda function and capturing the information you need instead.
/// @param header_name the name this column should have in the table
///
void AddMetric(std::function<std::string()> func, std::string header_name) {
Copy link
Member

Choose a reason for hiding this comment

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

take header_name by const&?

tests/config/SettingConfig.cpp Show resolved Hide resolved
queue_manager->AddRun(get_setting_config(), get_epochs());
queue_manager->AddNewQueuedRunToTable();

emp_assert(!queue_manager->IsEmpty());
Copy link
Member

Choose a reason for hiding this comment

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

Should we make these tests emp_always_assert so we don't have to worry about them getting turned off if we define NDEBUG?

tests/web/QueueManager.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants