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

✨ Added conversion functions for layouts of different coordinate types #125

Merged
merged 25 commits into from
Mar 2, 2023

Conversation

Drewniok
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Feb 12, 2023

Codecov Report

Merging #125 (23db248) into main (da61221) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
+ Coverage   93.87%   93.91%   +0.04%     
==========================================
  Files          75       75              
  Lines        6955     7002      +47     
==========================================
+ Hits         6529     6576      +47     
  Misses        426      426              
Impacted Files Coverage Δ
include/fiction/utils/layout_utils.hpp 97.64% <100.00%> (+2.40%) ⬆️
include/fiction/layouts/cell_level_layout.hpp 100.00% <0.00%> (ø)

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 da61221...23db248. Read the comment docs.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Collaborator

@marcelwa marcelwa left a comment

Choose a reason for hiding this comment

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

Thanks for the addition! Could you please address the following points:

  • Move both functions to /utils/layout_utils.hpp
  • Add respective static_asserts
  • Copy additional info stored in cell_level_layout to the newly created layout: inputs, outputs, cell modes, cell names, layout name
  • Add missing newlines at the end of each file

@marcelwa
Copy link
Collaborator

What happens to layouts based on SiQAD coordinates with negative values if they are to be converted to offset::ucoord_t coordinates?

@Drewniok
Copy link
Collaborator Author

Drewniok commented Feb 13, 2023

What happens to layouts based on SiQAD coordinates with negative values if they are to be converted to offset::ucoord_t coordinates?

I mean, the function to_fiction_coord will fail, right?

@Drewniok Drewniok closed this Feb 13, 2023
@Drewniok Drewniok reopened this Feb 13, 2023
@marcelwa
Copy link
Collaborator

marcelwa commented Feb 13, 2023

No, it won't. It has no method to "fail". It would produce coordinates that convert negative numbers into their respective unsigned representation, i.e., 2^{31} - 1 - c, where c is the SiQAD coordinate value.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

docs/utils/utils.rst Show resolved Hide resolved
include/fiction/traits.hpp Show resolved Hide resolved
include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
include/fiction/utils/layout_utils.hpp Outdated Show resolved Hide resolved
test/utils/layout_utils.cpp Outdated Show resolved Hide resolved
@marcelwa
Copy link
Collaborator

Many thanks for the updates. I've performed some restructuring and added failing tests. The original layout's aspect ratio is not yet copied to the converted layouts.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

3 similar comments
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@marcelwa marcelwa changed the title convert layouts from SiQAD coordinates to fiction and vs. ✨ Added conversion functions for layouts of different coordinate types Mar 2, 2023
@marcelwa marcelwa added the enhancement New feature or request label Mar 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

clang-tidy review says "All clean, LGTM! 👍"

@marcelwa marcelwa merged commit c77a7c9 into cda-tum:main Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants