Skip to content

Commit

Permalink
Add some global smart constructors to domain
Browse files Browse the repository at this point in the history
  • Loading branch information
yuriy committed Apr 3, 2017
1 parent cb5a62d commit f9bd821
Showing 1 changed file with 38 additions and 1 deletion.
39 changes: 38 additions & 1 deletion maple/Domain.mpl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,23 @@ Domain := module()
global DOMAIN; global DBound; global DConstrain; global DSum; global DSplit; global DInto; global DNoSol;

local ModuleLoad := proc($)
local ty_nm;
local ty_nm, g;
for ty_nm in [ indices(DomainTypes, nolist) ] do
TypeTools[AddType]( ty_nm, DomainTypes[ty_nm] );
end do;

#op([2,6], ...) of a module is its globals.
for g in op([2,6], thismodule) do
if g <> eval(g) then
unassign(g);
WARNING("Previous value of global name '%1' erased.", g)
end if;
if assigned(Domain:-GLOBALS[g]) then
assign(g = copy(Domain:-GLOBALS[g]));
end if;
protect(g);
end do;

unprotect(Domain:-ExtBound);
ExtBound[`Int`] :=
Record('MakeKB'=KB:-genLebesgue
Expand Down Expand Up @@ -343,6 +355,31 @@ Domain := module()
,(HDomain_mb = ''Or(HDomain, DomNoSol)'' )
] );

local GLOBALS := table(
['DOMAIN' =
(proc()
local errs := indets([args[2..-1]], DomNoSol);
if errs <> {} then
'procname'(args[1], 'DNoSol'(map(op,errs)[]));
end if;
'procname'(args);
end proc)
, 'DConstrain' =
(proc()
local as := {args};
if false in as then return DSum() end if;
as := remove(x->x::identical(false),as);
'procname'(op(as));
end proc)
, 'DSum' =
(proc()
local as := [args];
as := subsindets(as, specfunc(`DSum`), xs->
subsindets(xs, specfunc(`DSum`), op));
if nops(as) = 1 then return op(1,as) end if;
'procname'(op(as));
end proc) ]);

# Extending domain extraction and replacement.
export ExtBound := table();
export ExtShape := table();
Expand Down

14 comments on commit f9bd821

@ccshan
Copy link
Member

@ccshan ccshan commented on f9bd821 Apr 26, 2017

Choose a reason for hiding this comment

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

I don't see the purpose of the GLOBALS table and its associated machinery in ModuleLoad. Why not just assign procs to DOMAIN, etc. and export them? @JacquesCarette

@JacquesCarette
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really something that @yuriy0 should answer. This particular use seems like overkill. Other uses are 'clever' because it means that the update script does not need to be changed anytime new globals get added to Hakaru. I think the goal was to make reloading the library simpler, including making it actually work properly on Windows (which it never did unless you remembered to not have any active Maples when you did it).

@ccshan
Copy link
Member

@ccshan ccshan commented on f9bd821 Apr 26, 2017

Choose a reason for hiding this comment

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

I'm not sure what other uses (of what?) you have in mind. I'll remove this GLOBALS table for now. Thanks.

@JacquesCarette
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you don't break update-library, especially on Windows, when you do that. Thanks.

@ccshan
Copy link
Member

@ccshan ccshan commented on f9bd821 Apr 26, 2017

Choose a reason for hiding this comment

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

Why would it break update-library if I turn the GLOBALS table into exported assignments?

@JacquesCarette
Copy link
Contributor

Choose a reason for hiding this comment

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

Look at the new ModuleLoad code here and there, as well as the update-library. It uses that GLOBALS table directly, I believe.

@ccshan
Copy link
Member

@ccshan ccshan commented on f9bd821 Apr 26, 2017

Choose a reason for hiding this comment

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

Sorry, I don't know what you mean because I searched for the following regexps and didn't find anything:

GLOBALS
thismodule
2 *, *6

@JacquesCarette
Copy link
Contributor

Choose a reason for hiding this comment

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

Look at Hakaru.mpl and Domain.mpl. thismodule is manipulated in that way in both.

@ccshan
Copy link
Member

@ccshan ccshan commented on f9bd821 Apr 26, 2017

Choose a reason for hiding this comment

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

Are you talking about the calls to unassign, WARNING, protect, unprotect? I think I didn't disturb them...

@ccshan
Copy link
Member

@ccshan ccshan commented on f9bd821 May 12, 2017

Choose a reason for hiding this comment

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

This commit adds some globals, such as DConstrain, that need to be assigned definitions, as @yuriy0 explains at 4d4b28f#commitcomment-22123094

I think we should avoid globals that are assigned definitions. For globals whose names begin with eval/ or depends/ or print/, avoidance may be impossible. But for smart constructors such as DConstrain, I believe avoidance is possible and preferable. I understand from @JacquesCarette that the way I turned DConstrain and friends into definitions exported from the Domain module (cb91d54) may have made update-archive more difficult to use on Windows. However, I still have high hopes that this Windows difficulty can be eliminated, perhaps by moving the exported definitions of DConstrain and friends into a separate module that does not depend on Domain. I'm happy to try, but I need to be able to test update-archive on Windows, so I have made a pending request for a demo of update-archive on Windows (cb91d54#commitcomment-21934956).

@yuriy0
Copy link

@yuriy0 yuriy0 commented on f9bd821 May 12, 2017

Choose a reason for hiding this comment

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

I think we should avoid globals that are assigned definitions. For globals whose names begin with eval/ or depends/ or print/, avoidance may be impossible. But for smart constructors such as DConstrain, I believe avoidance is possible and preferable.

I agree.

I understand from @JacquesCarette that the way I turned DConstrain and friends into definitions exported from the Domain module (cb91d54) may have made update-archive more difficult to use on Windows.

This isn't the case (at least on my Windows machine) - the change in question may cause some (minor) issues, but they shouldn't be platform related.
The only Windows-specific issue related to update-archive is the fact that while a Maple worksheet is running in which Hakaru was loaded, the ppaml.mla file is "locked". I don't know if you're familiar with this particular 'feature' of Windows, but it will not allow the file to be modified by any process other than the one which locked it (in this case, Maple). A consequence is that FileTools:-Remove will not work, because this is making a system call to some other process. However, the Maple archive tool can happily delete individual entries of an archive, which combined with the ability to list all entries in the archive gives a way to get an empty archive without deleting the file. At least on my machine, this will work even when the archive is open in Maple, presumably because there is a single Maple server process which owns the Windows file lock on the archive. This is currently how it's done in UpdateArchive when the file itself cannot be deleted.

The only issue with this change is that uses Domain has to be inserted in all the correct places; but that can't occur in submodules of Domain. This is not a platform issue, but a Maple issue, or an issue with how I've structured the Domain module and submodules.

Perhaps coincidentally, the files inside of the maple/Domain/Improve directory aren't 'technically' submodules of Domain; in particular, using uses Domain is valid in these "submodules" of Domain and these only! And (certainly) coincidentally the places where cb91d54 added uses Domain are all inside Domain/Improve and are the only places where it is needed.

I need to be able to test update-archive on Windows, so I have made a pending request for a demo of update-archive on Windows

It is the change from global symbols to exported symbols which causes the need for uses Domain, so you should be able to test "moving the exported definitions of DConstrain and friends into a separate module that does not depend on Domain" regardless of anything which happens to update-archive. I think this is a good idea; and something which we should do for all of our "global constructors", even those which are not assigned definitions, e.g. Bind, Weight, Ret, Msum, Plate, Context, Pair, _Unit, PARTITION, all the primitive measures, all the Hakaru type constructors; that is, every global which is not one of the ones used as part of some Maple extension mechanism, like print/*, depends/*, etc.. I propose calling such a module HakaruGlobals or HakaruGlobalSymbols, and making it a package so one can write uses HakaruGlobals;.

I'm not sure what a demo of update-archive would consist of - I've been using the Makefile for a while now, and running make ppaml.mla on my (Windows) machine succeeds by producing no output, regardless of whether the archive exists or not. Running Hakaru:-UpdateArchive(); in a worksheet also succeeds by producing no output; this requires that there already be a Hakaru archive.

I don't know how to test whether I have preserved 'a clean "build" process that worked both for bootstrap and for incremental updates, on all 3 platforms'

The only way to reliably test "success" or "failure" of this operation is to run all of the test cases; or at least a sufficient number of them to hit all the functions present in Hakaru. For example, if you were to somehow break the update of the Partition module, it would no longer be loaded, but tests which do not use Partition would still work. Perhaps we need a new test file consisting of test cases specifically chosen to hit all of the functionality (literally, as in all of the functions) of Hakaru, for testing that update-archive succeeded?

Whether we are in 'bootstrap' or 'incremental' mode shouldn't matter - the same function is being run in either case. The only difference will be on Windows, when the archive already exists; but whether the archive file is deleted and a new empty archive is created, or the existing archive file is emptied to produce a new empty archive, should not make a difference. A potential way to test that the previous statement is actually true is to remove the logic which deletes the archive file, and just keep the logic which clobbers each entry in the archive individually.

@JacquesCarette
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the thorough reply. Are there some Action Items that still need to be done from this?

@yuriy0
Copy link

@yuriy0 yuriy0 commented on f9bd821 May 20, 2017

Choose a reason for hiding this comment

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

Are there some Action Items that still need to be done from this?

If github issues count, then #65 and #66.

@JacquesCarette
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

Please sign in to comment.