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

Adding function to read xml file to dot file, along with a topology.x… #31

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

sappyb
Copy link
Contributor

@sappyb sappyb commented Jun 13, 2019

…ml file which is a quartz topology and test.xml which is a small topology for testing

…ml file which is a quartz topology and test.xml which is a small topology for testing
@bhatele bhatele requested a review from ggeorgakoudis June 18, 2019 07:42
@bhatele bhatele assigned nikhil-jain and unassigned nikhil-jain Jun 18, 2019
@bhatele bhatele requested a review from nikhil-jain June 18, 2019 07:42
@bhatele
Copy link
Member

bhatele commented Jun 18, 2019

@Saptarshibh these files are not in the right place. The script and test topology file should be under utils/. The full quartz topology file shouldn't be checked in. Can you make that change?

@sappyb
Copy link
Contributor Author

sappyb commented Jun 18, 2019 via email

def writeDot(self, switchRadix=48, nodeTags='FIs', switchTags='Switches', write=True, **kwargs):
tree = ET.parse(self.fullFilePath)
root = tree.getroot()
terminalIndex = 274877906944
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to hex and add a comment on the format

tree = ET.parse(self.fullFilePath)
root = tree.getroot()
terminalIndex = 274877906944
switchIndex = 12884901888
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to hex and add a comment on the format

self.levelsOfSwitches = None
self.fullFilePath = fullFilePath

def writeDot(self, switchRadix=48, nodeTags='FIs', switchTags='Switches', write=True, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should switchRadix be using a default value?

@ggeorgakoudis
Copy link
Contributor

Hi, added some comments in file. Also, does the implementation handle any arbitrary graph or is it only for fat-tree variants?

@sappyb
Copy link
Contributor Author

sappyb commented Jun 18, 2019 via email

@sappyb
Copy link
Contributor Author

sappyb commented Jun 18, 2019 via email

@sappyb
Copy link
Contributor Author

sappyb commented Jun 18, 2019 via email

@ggeorgakoudis
Copy link
Contributor

Saptarshi, next time, it would be helpful to answer the comments in code in their chat window.

Could you document what tests you did? Do those tests cover most/all of the expected functionality? Could you try a non fat-tree topology?

@bhatele bhatele removed the request for review from nikhil-jain November 6, 2020 17:53
@bhatele bhatele changed the base branch from master to develop November 19, 2020 13:13
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.

5 participants