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

Fix paper issues seen in review #47

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

Conversation

MathisFederico
Copy link
Collaborator

No description provided.

Copy link

codacy-production bot commented Aug 31, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.00% (target: -1.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (805c671) 3746 3501 93.46%
Head commit (84ce614) 3746 (+0) 3501 (+0) 93.46% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#47) 0 0 ∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

@MathisFederico
Copy link
Collaborator Author

MathisFederico commented Sep 8, 2024

This PR aims to fix the paper issues mentioned in #45:

  • line 7, "Hierarchical reasoning poses a fundamental challenge in the field of artificial intelligence." --- please cite any sources on hierarchical reasoning and its AI challenges
  • line 8, "Existing methods may struggle when confronted with hierarchical tasks" --- please cite papers confirming this claim
  • lines 8-9, "there is a scarcity of suitable environments or benchmarks designed to comprehend how the structure of the underlying hierarchy influence a task difficulty" --- I haven't found an explanation how HierarchyCraft helps to comprehend influence of hierarchy structure on task difficulty.
    ---> Make it more clear that HierarchyCraft allows to build environments with various hierarchical structures to see how they influence learning a behavior on it.
  • line 14-15, "tasks ... that do not necessitate feature extraction. This includes tasks containing pixel images, text, sound" --- I can't help reading it as "tasks that do not necessitate feature extraction include tasks containing images etc". Could you please reformulate these two sentence to make them less ambiguous?
  • line 15-16 "or any data requiring deep-learning based feature extraction" --- I agree that deep-learning is a go-to method for feature extraction nowadays, but I don't think that any data requires it. Could you please reformulate?
  • line 24, "current hierarchical benchmarks often limit themselves to a single hierarchical structure per benchmark" --- HierarchyCraft is compared to RL benchmarks, but is it a benchmark itself? I don't see such a statement anywhere in a paper
    ---> It is not one yet, but its a tool allowing to create some and explore what would make a good hierarchical benchmark and what metrics could be used in order to quantify this "hierarchical difficulty"
  • line 52, "a undeniably complex hierarchical structure" --- probably "an undeniably"
  • lines 52-53, "this underlying hierarchical structures is fixed" --- probably "these underlying hierarchical structures are fixed"
  • line 63, "e.g., Swords " --- is the capital letter really needed here?
  • line 64, "easier.), " --- probably the full stop is unnecessary
  • line 87, "But each Transformations has" --- probably "each Transformation" or "each of Transformations"
  • line 88, "(eg. have" --- it's probably better to use consistent abbreviations through the paper, and you use "e.g." in other cases
  • line 88, "enought" --- probably "enough"
  • line 90, "HierarchyCraft directly provides a low-dimensional latent representation that does not require learning, as depicted in Figure 5." --- I don't see how representations in Figure 5 are latent. Could you please explain or drop the word "latent"?
  • line 100, "This not only saves computational time" --- I would suggest highlighting your contribution of a library of environments in HierarchyCraft. If I got it right, one has to code the transformations by hand to avoid representation learning. So, for included environments, it's an important contribution in my opinion, but the reader should also be warned that if they want to add environments of their own, they will have to do this job themselves. The framework's design won't do it for them automatically.

General remarks to answer / fix:

  • The title promises to talk about benchmarking with HierarchyCraft but it doesn't seem to happen. Instead, you call HierarchyCraft "a lightweight environment builder". I understand that "a set of pre-defined hierarchical environments" that you mention is indeed a benchmark, but I would like it to be clearly stated.
    ---> Maybe we should change the title to "One step towards well quantified hierarchical benchmarks with HierarchyCraft"
  • From the paper, I don't learn anything about the environments available in HierarchyCraft. I think it's important to mention them, in particular because creation of requirements graphs for them is exactly your contribution that helps other researchers to skip representation learning part and focus on study of hierarchical structures per se. On the other hand, descriptions of existing benchmarks might be shortend if needed to keep the paper to JOSS size standards.
    ---> Add small description of basic hierarchical structure builders (Tower, Recursive, Random) and fixed one imitating other environments (MineHCraft, MiniHCraft)
  • Please double check the citations and add DOIs as highlighted by the editorial bot. For example, I see that the citation for MiniGrid is not the one recommended (https://github.com/Farama-Foundation/Minigrid?tab=readme-ov-file#citation).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants