Skip to content

Conversation

@MathisFederico
Copy link
Collaborator

No description provided.

@codacy-production
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 (2672bef) 3628 3381 93.19%
Head commit (fbf88f1) 3628 (+0) 3381 (+0) 93.19% (+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).

@MathisFederico MathisFederico changed the base branch from master to Switch-to-gymnasium-for-rl-env-compatibility January 12, 2025 15:58
Base automatically changed from Switch-to-gymnasium-for-rl-env-compatibility to master January 12, 2025 16:12
@MathisFederico MathisFederico merged commit 16ad417 into master Jan 20, 2025
15 checks passed
@MathisFederico MathisFederico deleted the paperfix/shang branch January 20, 2025 18:43
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.

3 participants