Skip to content

Adding support for direct problem loading#80

Open
supersimple33 wants to merge 8 commits into
jvkersch:mainfrom
supersimple33:main-clone
Open

Adding support for direct problem loading#80
supersimple33 wants to merge 8 commits into
jvkersch:mainfrom
supersimple33:main-clone

Conversation

@supersimple33

Copy link
Copy Markdown
Contributor

Hey I had a use case where I needed to skip the file-saving step of the process and so I wrote this PR which allows one to leverage the tri2dat function of Concorde via this python module.
This is just #77 but with its dedicated branch now

no more anything

no more class

drop import

add back

bring back underscore

debug

changing responsibility

adding a getter

add axes

new numpy conv

new way

bug

making rows

hopefully final

new way

final updates

dont flip back

Revert "dont flip back"

This reverts commit 449077d.
@supersimple33

Copy link
Copy Markdown
Contributor Author

@jvkersch Let me know what you think about this

@jvkersch jvkersch left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@supersimple33 Thank you for your contribution, this looks very promising!

Just a few comments inline (in general, it is good to keep the PR limited to the actual change -- stylistic changes are preferably done separately). Also, could you add a unit test to exercise this functionality

Comment thread concorde/_concorde.pyx
def adj(self):
cdef int[:, :] adj_data
if self.initialized and self.c_data.adj != NULL:
adj_data = np.zeros((self.ncount, self.ncount), dtype=np.int32)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This can be np.empty -- you're filling in the entries of the array later on anyway, so no need to zero initialize it now.

Comment thread concorde/_concorde.pyx
Comment on lines +79 to +86
for i in range(self.ncount):
for j in range(self.ncount):
if i > j:
adj_data[i, j] = self.c_data.adj[i][j]
elif i < j:
adj_data[i, j] = self.c_data.adj[j][i]
else:
adj_data[i, j] = 0

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think this can be done a little more tightly:

for i in range(self.ncount):
    adj_data[i, i] = 0
    for j in range(i + 1, self.ncount):
        adj_data[i, j] = adj_data[j, i] = self.c_data.adj[i, j]

Please double-check though...

Comment thread concorde/_concorde.pyx
Comment on lines +107 to +109
cdef _CCdatagroup dat

dat = _CCdatagroup()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can we combine this as cdef _CCdatagroup dat = _CCdatagroup()?

Comment thread concorde/concorde.py

solution = Solution.from_file(tmp / "problem.sol", output=res.stdout)
return solution
return Solution.from_file(tmp / "problem.sol", output=res.stdout)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is not related to the current PR, it would be better to revert this. (topical diffs make for better traceability in the git history)

Comment thread concorde/concorde.py
Comment on lines -67 to +66
if pyconcorde_binaries.exists():
# Git checkout, with pyconcorde-build as git subtree
location = _PLATFORM_MAP[(platform.system(), platform.machine())]
concorde_exe = pyconcorde_binaries / location / "concorde"
else:
if not pyconcorde_binaries.exists():

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is not related to the current PR, it would be better to revert this.

Comment thread concorde/concorde.py
Comment on lines -74 to +72
concorde_exe = project_dir / "concorde" / "concorde"
return concorde_exe
return project_dir / "concorde" / "concorde"
# Git checkout, with pyconcorde-build as git subtree
location = _PLATFORM_MAP[(platform.system(), platform.machine())]
return pyconcorde_binaries / location / "concorde"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is not related to the current PR, it would be better to revert this.

Comment thread setup.py
'CFLAGS="{cflags}" ./configure --prefix {data} '
"--with-qsopt={data} {flags}"
).format(cflags=cflags, data=datadir, flags=flags)
cwd = f'CFLAGS="{cflags}" ./configure --prefix {datadir} --with-qsopt={datadir} {flags}'

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is not related to the current PR, it would be better to revert this.

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